Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "tools: add install_yq" #1400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

SamYuan1990
Copy link
Collaborator

Reverts #1393

There seems no need for a PR to add yq.
In CI if we have grafana features, which yq installed at https://github.com/sustainable-computing-io/local-dev-cluster/blob/main/main.sh#L190 this covers case which we build up instance on a new created ec2.

and if we need to handle json, yq and jq been installed via GHA naivety.
https://github.com/actions/runner-images/blob/7bb1d84f7071bfa9c350d7552d9631d9a69bfdb0/images/ubuntu/Ubuntu2404-Readme.md?plain=1#L72-L86

So, what's the reason we add yq install in our code?

@SamYuan1990 SamYuan1990 requested a review from rootfs May 3, 2024 02:27
@@ -291,7 +291,6 @@ TOOLS = govulncheck \
jq \
kubectl \
kustomize \
yq \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing it, how about we maintian a different target called ci-tools and have

tools: ci-tools 

YQ is quite useful since we have a lot of yaml files in manifests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for a new target and ci-tools is ok - but it doesn't encompass the development side.

could we not just have a deps target that installs all the depdendencies and centralise all the dependencies/tools between the kepler repo and the local-dev-cluster in one repo...

From a developer experience POV - it would make the onboarding experience a lot smoother IMHO. Right now we rely on the developer to manually run a command to install the tools they need after finding the appropriate reference in the Readme rather than automating this step

@maryamtahhan
Copy link
Contributor

maryamtahhan commented May 3, 2024

So, what's the reason we add yq install in our code?

To improve developer experience. If YQ isn't installed then local-dev-cluster will skip the grafana dashboard automation step and the developer will need to manually import the dashboard.

TBH as a developer it would be great if the first thing I could do after cloning the kepler repo is run make deps or some other target that installs all the tools that I need to deploy the full stack. I don't see a place where the local-dev-cluster pre-requisites are automated for developer experience and we rely on the (new) developer to run a bunch of steps to get up and running with kepler.

IMHO - the kepler developer onboarding experience could be greatly improved.

@rootfs
Copy link
Contributor

rootfs commented May 3, 2024

I tested jq and yq on my end, for the use case kepler needs (i.e. parsing kubectl output), both can do the job. Since we only need one, what about replacing existing jq reference with yq?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants