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.

Sign up for GitHub

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

Jump to bottom

DOC: Advocate for using action from tagged release commit shas #13

Merged
merged 3 commits into from
Jun 8, 2023
Merged

DOC: Advocate for using action from tagged release commit shas #13

merged 3 commits into from
Jun 8, 2023

Conversation

matthewfeickert
Copy link
Member

Resolves #7

  • For security best practices, advocate that users of the action use it from known commit shas that correspond to tagged releases.
  • Advocate that users use a Dependabot config file to update the action on new tags. This will bump the commit sha and also bump the release tag in the comment of the commit sha.

@matthewfeickert matthewfeickert added the documentation Improvements or additions to documentation label May 31, 2023
@matthewfeickert matthewfeickert self-assigned this May 31, 2023
bsipocz
bsipocz reviewed May 31, 2023
View reviewed changes
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

The notion sounds fine to me, and we talked about doing it anyway while writing the action, but first just wanted to get something out of the door asap.

README.md
@@ -11,12 +11,25 @@ jobs:
steps:
...
- name: Upload wheel
uses: scientific-python/upload-nightly-action@main
uses: scientific-python/upload-nightly-action@8f0394fd2aa0c85d7364a9958652e8994e06b23c # 0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

could this be a major version instead, the same we do for checkout, or python, or etc?

Copy link
Member

Choose a reason for hiding this comment

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

It depends on how much you trust the project; releases / tags can be removed and replaced, while SHAs are a bit harder to fake. But if you trust official releases made by the org, and you just want to review the new action to make sure nothing big changed, versions are fine.

Copy link
Member

Choose a reason for hiding this comment

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

well, I would rather like to think that the releases made in the scientific-python org are official and trustworthy :)

Copy link
Member

Choose a reason for hiding this comment

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

(after all, we all use a lot of main actions, e.g. the artifact upload one, which is I agree is not super ideal as a best practice)

Copy link
Member

Choose a reason for hiding this comment

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

In that case, if you trust that bunch, versions seem fine ;)

Copy link
Member Author

@matthewfeickert matthewfeickert Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that @ksunden gave a good and concise summary in matplotlib/matplotlib#26023 (comment). This is not about trusting orgs or not trusting orgs. It is about trying to be extra security cautious for anything that is publishing distributions to any package index. Is this overkill? Yeah, for sure in my mind, but it also seems like a pattern with no downsides other than you have to have your eyes track a little further across the screen to read the associated tag.

after all, we all use a lot of main actions, e.g. the artifact upload one, which is I agree is not super ideal as a best practice

I will fully agree with you though that tags are better than main, and patch version tags are better than major version tags. Though I'm also willing to agree that there should be additional security hardening than using a latest tag Docker image

upload-nightly-action/Dockerfile

Line 1 in dbd5a4d

FROM continuumio/miniconda3:latest

and so using the SHAs only really helps later releases (0.1.0 is the first commit in the repo). Also this GHA has no CI at all yet. 😬

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed reply. To make this bikeshed really shiny, could I ask you to include some of Kyle's concise commentary as a comment in this readme? Or even just the link you have above would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I'm in meetings all day today though (on phone now) so feel free to add with the "Add a suggestion" tools and then accept and merge the suggestion if people would like this to get done before tomorrow.

README.md Show resolved Hide resolved
bsipocz
bsipocz approved these changes Jun 2, 2023
View reviewed changes
README.md
@@ -11,12 +11,25 @@ jobs:
steps:
...
- name: Upload wheel
uses: scientific-python/upload-nightly-action@main
uses: scientific-python/upload-nightly-action@8f0394fd2aa0c85d7364a9958652e8994e06b23c # 0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed reply. To make this bikeshed really shiny, could I ask you to include some of Kyle's concise commentary as a comment in this readme? Or even just the link you have above would be useful.

README.md Show resolved Hide resolved
@matthewfeickert matthewfeickert mentioned this pull request Jun 2, 2023
Closed
@matthewfeickert
DOC: Advocate for using action from tagged release commit SHAs
186268f 
* For security best practices, advocate that users of the action use it
  from known commit SHAs that correspond to tagged releases.
* Advocate that users use a Dependabot config file to update the action
  on new tags. This will bump the commit SHA and also bump the release
  tag in the comment of the commit SHA.
   - c.f. https://learn.scientific-python.org/development/guides/gha_basic/#updating
@matthewfeickert
DOC: Fix typo
7d347c3 
* 'Intregration' -> 'Integration'
matthewfeickert
matthewfeickert commented Jun 5, 2023
View reviewed changes
README.md Outdated
Comment on lines 9 to 12
<!-- c.f. https://github.com/scientific-python/upload-nightly-action/pull/13 and
https://github.com/matplotlib/matplotlib/pull/26023#discussion_r1212539700
for short summary of why using commit SHA -->

Copy link
Member Author

Choose a reason for hiding this comment

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

@bsipocz ...and after a long delay given too many meetings and weekend travel this is in now. Look good?

@jarrodmillman
Copy link
Member

Feel free to follow-up on this in a future PR, but this looks like it is already a good improvement. Thanks!!

@jarrodmillman jarrodmillman merged commit 468089a into scientific-python:main Jun 8, 2023
@matthewfeickert matthewfeickert deleted the docs/recommend-use-of-tags branch June 8, 2023 16:00
@jarrodmillman jarrodmillman added this to the 0.2.0 milestone Jun 8, 2023
This was referenced Jan 24, 2024
Merged
Merged
@matthewfeickert matthewfeickert mentioned this pull request Oct 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanv stefanv stefanv approved these changes

@jarrodmillman jarrodmillman jarrodmillman approved these changes

@bsipocz bsipocz bsipocz approved these changes

@Carreau Carreau Awaiting requested review from Carreau

@martinfleis martinfleis Awaiting requested review from martinfleis

Assignees

@matthewfeickert matthewfeickert

Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.

Consider not advocating using action from main
4 participants
@matthewfeickert @jarrodmillman @stefanv @bsipocz