search

ProjectPythia / pythia-foundations

Jupyterbook source for the Foundations collection
http://foundations.projectpythia.org
Apache License 2.0
61 stars 43 forks source link

Fix netlify action #201

Closed brian-rose closed 2 years ago

brian-rose commented 2 years ago

Bump version of https://github.com/nwtgck/actions-netlify since dependabot hasn't done it for us. Hopefully this addresses https://github.com/ProjectPythia/pythia-foundations/issues/197

github-actions[bot] commented 2 years ago

This pull request is being automatically built with GitHub Actions and Netlify. To see the status of your deployment, click below.

🔍 Git commit SHA: 7d2482cf36afc5cb69056cac533a5e1b51ee5c47 ✅ Deployment Preview URL: https://61dcbb535b7a3b2e1171e727--pythia-foundations.netlify.app

brian-rose commented 2 years ago

Hopefully this works! Seems easy enough to reverse if it doesn't.

The preview works for this PR. I think the preview uses the updated action instructions that are in this PR so we should be good.

dopplershift commented 2 years ago

Looking at the upstream repo, it looks like currently both v1.2 and v1.2.3, so this PR shouldn't be actually affecting anything. Do we prefer in general to allow upstream to manage (by updating their v1.x tag) or manage ourselves with Dependabot?

brian-rose commented 2 years ago

Looking at the upstream repo, it looks like currently both v1.2 and v1.2.3, so this PR shouldn't be actually affecting anything. Do we prefer in general to allow upstream to manage (by updating their v1.x tag) or manage ourselves with Dependabot?

I'd rather let Dependabot handle things, as it almost always does things better than me. But thus far Dependabot hasn't proposed any changes to the action identified as problematic in #197. Is the issue here that the upstream fix wasn't labeled as v1.3, and actions doesn't look past the second digit?

It's not clear to me how to actually see which version of an action is being used each time.

kmpaul commented 2 years ago

I agree with @dopplershift. I think this is an upstream issue. If we need to fix this now, then I'm fine bumping this manually, but we should change it back at some point.

dopplershift commented 2 years ago

@brian-rose The general idea with action versioning is that:

  1. You can pin the exact version, and rely on manual/dependabot to move; e.g. v1.2.2->v1.2.3.
  2. You can pin down to some level, like v1 or v1.2. Upstream maintainers handle making sure that when they make a new release and things are considered "stable", that the v1 tag itself is updated (moving from v1 pointing to v1.2.2 to v1.2.3); same for v1.2. This is transparent to us and dependabot only issues updates when there is a new tag at that level. So we won't see a dependabot PR (or need one) until there's a v1.3.

My opinion right now is that unless this PR is demonstrated to fix something--meaning other PRs are currently failing--we don't merge this since it's not actually changing the action we're running right now, it's just moving us to a more exact version. The v1.2 and v1.2.3 are currently pointing to the exact same code for the action, so this change should be a complete NOOP.

brian-rose commented 2 years ago

@dopplershift Thanks, I think I get it, and I'm happy to close this PR without merging.

It's just still not clear to me whether the security issue in #197 was fixed upstream with the release of v1.2.3 (in which case we should just close #197), or not, in which case I'm not sure what action we should take (if any).

dopplershift commented 2 years ago

I commented on #197, but I just verified that the 1.2 tag now contains the referenced commit.

brian-rose commented 2 years ago

Excellent, thanks. I will close this PR without merging.