Skip to content

Conversation

@peterbe
Copy link
Owner

@peterbe peterbe commented Oct 28, 2024

No description provided.

@peterbe
Copy link
Owner Author

peterbe commented Oct 28, 2024

@hartwork r?

Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@peterbe the pull request seems to do three things:

  1. unpin action actions/checkout
  2. change string quoting
  3. adjust indentation

Let me address them one by one:

  • (1) reduces pull request noise but takes away robustness and security (unless you consider actions/checkout a place that you trust as much as yourself). I would personally go the opposite direction and pin all actions that are not yet pinned to a SHA1 to a SHA1 for max security and consistency with the others.
  • (2) is of arguable value to me buw why not. Should be a at least separate commit though, if not a separate pull request.
  • (3) same as (2).

@peterbe
Copy link
Owner Author

peterbe commented Oct 28, 2024

@peterbe the pull request seems to do three things:

  1. unpin action actions/checkout
  2. change string quoting
  3. adjust indentation

Let me address them one by one:

  • (1) reduces pull request noise but takes away robustness and security (unless you consider actions/checkout a place that you trust as much as yourself). I would personally go the opposite direction and pin all actions that are not yet pinned to a SHA1 to a SHA1 for max security and consistency with the others.

I trust actions/checkout and actions/setup-python to a significant degree. Much more than something like somedude/my-cool-thing.

The other significant difference is that these are not used for shipping something into a release or a production system. They're just use for testing the code.
It'd be different if you used GitHub Actions for something like pushing a docker built image into a production cluster.

  • (2) is of arguable value to me buw why not. Should be a at least separate commit though, if not a separate pull request.
  • (3) same as (2).

These were due to my "GitHub Actions Workflow" extension in VS Code. It can lint and format the YAML.

@hartwork
Copy link
Collaborator

@peterbe I cannot approve this pull request because of C9. Please either make well separated commits or be okay without my approval.

@peterbe
Copy link
Owner Author

peterbe commented Oct 28, 2024

@peterbe I cannot approve this pull request because of C9. Please either make well separated commits or be okay without my approval.

Fair!
#202

I subscribe to the same rule when it's a work-related PR where more is at stake. I shouldn't have been lazy.

@peterbe peterbe requested a review from hartwork October 28, 2024 14:57
Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@peterbe Thanks for splitting #202 off of this!

I'm approving now based on…

  • your conscious decision to fully trust future changes to actions/checkout over time
  • my hope that you will either squash the three commits before merge or use squash merge for this pull request 🙏

@peterbe peterbe merged commit 3c7a3ed into master Oct 28, 2024
10 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.

3 participants