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

ci: refactor github workflows for readability #192

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Apr 5, 2024

This is just refactoring for readability. I looked at the ci.yaml workflow to learn how you installed jsonnet from a github action's workflow, but saw potential to contribute in order to make especially the other github workflow more readable.

@consideRatio consideRatio requested a review from a team April 5, 2024 20:06
@CLAassistant
Copy link

CLAassistant commented Apr 5, 2024

CLA assistant check
All committers have signed the CLA.

Comment on lines -16 to -20
- name: Setup PATH
shell: bash
run: |
mkdir -p "$HOME/.local/bin"
echo "$HOME/.local/bin" >> $GITHUB_PATH
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.local/bin is on path already, the folder doesn't exists though but can be created simply later.


- name: Install jsonnet
shell: bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The addition of shell: bash is relevant for github action definitions, but not in a github workflow file - the default is to use bash.

Comment on lines -25 to -28
wget https://github.com/google/go-jsonnet/releases/download/v${JSONNET_VERSION}/go-jsonnet_${JSONNET_VERSION}_Linux_x86_64.tar.gz
tar --extract --file=go-jsonnet_0.20.0_Linux_x86_64.tar.gz jsonnet
mv jsonnet "$HOME/.local/bin"
chmod u+x "$HOME/.local/bin/jsonnet"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By piping into tar, we don't create a file to clutter the working dir. Whats extracted are multiple files as well, not just jsonnet - also jsonnet-lint for example, and they are packaged with chmod +x already in the tar so they extract like that as well.

@@ -0,0 +1,25 @@
name: Publish docs via GitHub Pages
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This entire file was renamed from main.yml and with loads of quotes etc making it hard to read YAML.

I updated the mixed use of "build" "deploy" "publish" to just "publish".

@consideRatio consideRatio changed the title ci: update github workflows for readability ci: refactor github workflows for readability Apr 5, 2024
Comment on lines 6 to 13
tests:
runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
jsonnet-version: ["0.20.0"]
jsonnet-bundler-version: ["0.5.1"]
Copy link
Contributor Author

@consideRatio consideRatio Apr 5, 2024

Choose a reason for hiding this comment

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

By not declaring name explicit for the job as before, these versions can show up in the name. If multiple variants of this job are run at some point, it will be helpful to not declare name explicitly and omitting the matrix parameters for the jobs.

Copy link
Member

Choose a reason for hiding this comment

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

There is no plan to run this against multiple jsonnet and jb versions, I'd prefer to not turn this into a matrix and keep it a variable at the beginning of the file.

Copy link
Member

@Duologic Duologic left a comment

Choose a reason for hiding this comment

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

Looks fine in general, left one change request.

Comment on lines 6 to 13
tests:
runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
jsonnet-version: ["0.20.0"]
jsonnet-bundler-version: ["0.5.1"]
Copy link
Member

Choose a reason for hiding this comment

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

There is no plan to run this against multiple jsonnet and jb versions, I'd prefer to not turn this into a matrix and keep it a variable at the beginning of the file.

@consideRatio
Copy link
Contributor Author

Thank you for the quick review @Duologic - I pushed a commit going back to specifying env variables.

@Duologic Duologic merged commit 46b316d into grafana:main Apr 10, 2024
2 checks passed
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

3 participants