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

Improve the ordering of GHA workflow fields #399

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

deimantastumas
Copy link
Contributor

@deimantastumas deimantastumas commented Oct 5, 2021

Summary

When building the YAML file (GHA workflow) using the workflow structs defined in ./octo/actions/**, IF conditional is placed above the step name. This behaviour is not optimal as it's much easier to read workflows where conditionals are used below the step name. This can be easily fixed by switching the fields in the defined structs.

Examples of workflows used in acknowledged repositories where conditional is placed below the name, proving the common practice:

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@deimantastumas deimantastumas requested a review from a team October 5, 2021 11:13
@dmikusa
Copy link
Contributor

dmikusa commented Oct 5, 2021

Thanks for the PR. I'm not opposed to making this change, but I need to do some testing and see how this impacts our pipelines. I assume that it shouldn't change any functionality, just the readability of the YAML.

In the meantime, I would also suggest that you add comments above the struct explaining the struct (maybe how it maps to the GHA YAML) and making a note that order is intentional for the reason you shared. Just a sentence or two should be plenty. Otherwise, I could see someone accidentally changing this in the future, or possibly a formatter reshuffling them.

@deimantastumas
Copy link
Contributor Author

Addressed your review. Thank you!

@dmikusa dmikusa added semver:patch A change requiring a patch version bump type:enhancement A general enhancement labels Oct 7, 2021
@dmikusa dmikusa merged commit 14e2357 into paketo-buildpacks:main Oct 7, 2021
This was referenced Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants