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

First commits #1

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from
Open

First commits #1

wants to merge 27 commits into from

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Feb 24, 2025

🗣 Description

This pull request contains the first commits for a new skeleton for GitHub composite actions.

💭 Motivation and context

We need a skeleton of this type. See, for example, cisagov/action-disable-apparmor and cisagov/action-job-preamble.

🧪 Testing

All automated tests pass.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Finalize version.

✅ Post-merge checklist

  • Create a release.

@jsf9k jsf9k added the documentation This issue or pull request improves or adds to documentation label Feb 24, 2025
@jsf9k jsf9k self-assigned this Feb 24, 2025
@jsf9k jsf9k marked this pull request as ready for review February 24, 2025 15:22
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

LGTM, aside from one documentation question.

@jsf9k jsf9k requested a review from a team February 24, 2025 17:58
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Seems straightforward enough. 👍

version.txt Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we may benefit from having a tag.sh that is used solely to generate the major (vX) and minor (vX.Y) tags based on the full version tag that would generated when creating a release (if we don't want the script to just create all three and then just select the full version tag when creating the release).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about that too. This afternoon I started creating a release workflow that will create those tags automatically when a release is created.

Copy link
Member Author

@jsf9k jsf9k Feb 26, 2025

Choose a reason for hiding this comment

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

Please see commits 4156a58 through 666579b, as well as the following GitHub Actions runs:

Once @mcdonnnj resolves this conversation I will do a git rebase to clean up the commit history, remove all the prereleases and tags generated during testing, etc. I think that stuff muddies the history for future folks who review the commits in this repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if @cisagov/vm-dev folks have any opinions as to my use of zyactions/semver versus any of the other GH Actions listed here please let me know. There are other actions listed there that do the same thing.

These are unnecessary since the trigger events guarantee that the
reference type will always be a tag.
Otherwise the git push origin --delete command will fail.
This was done only for testing purposes (so I could trigger the
workflow by creating prereleases) but now testing is complete.
@jsf9k jsf9k requested a review from dav3r February 26, 2025 19:13
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

This looks pretty solid. Just a few minor suggestions for your consideration.

Comment on lines +70 to +72
- id: checkout-code
name: Checkout the code
uses: actions/checkout@v4
Copy link
Member

Choose a reason for hiding this comment

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

Another consistency one.

Suggested change
- id: checkout-code
name: Checkout the code
uses: actions/checkout@v4
- uses: actions/checkout@v4

Copy link
Member Author

Choose a reason for hiding this comment

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

I have been trying to add an id and name for every step since I started working with the GH Actions jazz recently. What is your reasoning for not doing that?

Copy link
Member

@mcdonnnj mcdonnnj Feb 26, 2025

Choose a reason for hiding this comment

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

I don't have a reason to not do that, but we should start with cisagov/skeleton-generic and push it down in a consistent manner. This is specifically because we haven't done it with uses of actions/checkout.

Copy link
Member Author

Choose a reason for hiding this comment

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

My intent was to take care of this inconsistency when I'm able to start using reusable workflows via cisagov/github-actions-workflows. In the meantime I'd like to continue using name and id in the new code I am creating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This issue or pull request improves or adds to documentation
Projects
Status: Reviewer approved
Development

Successfully merging this pull request may close these issues.

4 participants