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

ci/d: Use version instead of hash #709

Closed
wants to merge 1 commit into from

Conversation

Geod24
Copy link
Contributor

@Geod24 Geod24 commented Nov 17, 2020

There are plenty of other actions in this repository using a version
that is not certified by Github. The disclaimer makes this obvious.
The action in question is not simply a user's action, but maintained
by dlang-community, which is overseen by the D Language Foundation.
The recent set of changes to actions means that this workflow is currently
broken, as it was previously using add-path and set-env.
Keeping on upgrading the hash will provide a terrible user experience,
as it is likely to be forgotten, and will also require user-intervention
when another set of change breaks this action, hence why this updates
the hash to a version instead of changing it to the latest hash.

I note that the contributing guidelines have evolved to make the hash a recommendation rather than a requirement, thanks for that:

  • This workflow must only use actions that are produced by GitHub, in the actions organization, or
  • This workflow must only use actions that are produced by the language or ecosystem that the workflow supports. These actions must be published to the GitHub Marketplace. We recommend that these actions be referenced using the full 40 character hash of the action's commit instead of a tag.

@Geod24
Copy link
Contributor Author

Geod24 commented Nov 23, 2020

CC @andymckay

@Geod24
Copy link
Contributor Author

Geod24 commented Dec 2, 2020

Rebased. Ping @andymckay .

@Geod24 Geod24 requested a review from a team December 10, 2020 11:19
@Geod24
Copy link
Contributor Author

Geod24 commented Dec 10, 2020

Rebased on main. Ping @joshmgross @andymckay .

@@ -17,7 +17,7 @@ jobs:

steps:
- uses: actions/checkout@v2
- uses: dlang-community/setup-dlang@7c3e57bdc1ff2d8994f00e61b3ef400e67d2d7ac
- uses: dlang-community/setup-dlang@v1
Copy link
Member

Choose a reason for hiding this comment

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

Could we keep the SHA hash and leave a comment letting users opt-in to pinning to a version/branch?
Similar to the Ruby workflow

Suggested change
- uses: dlang-community/setup-dlang@v1
# To automatically get bug fixes and new versions for dlang-community/setup-dlang,
# change this to:
# uses: dlang-community/setup-dlang@v1
- uses: dlang-community/setup-dlang@4c99aa991ce7d19dd3064de0a4f2f6b2f152e2d7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping on upgrading the hash will provide a terrible user experience,
as it is likely to be forgotten, and will also require user-intervention
when another set of change breaks this action, hence why this updates
the hash to a version instead of changing it to the latest hash.

As explained in the commit message, I think this provides a terrible user-experience. Most users, if not all, will want the auto-updating behavior, which the Github team even acknowledge with their guidelines of have v1 branch.
Having a comment to tell user to do the thing they want just relegates community-maintained languages, or languages Github has little interest in, to a back-seat, as obvious from the time this PR (and its predecessor) has been open.

@Geod24
Copy link
Contributor Author

Geod24 commented Dec 29, 2020

Rebased on main.

@Geod24
Copy link
Contributor Author

Geod24 commented Jan 2, 2021

Ping @joshmgross

Copy link
Member

@joshmgross joshmgross left a comment

Choose a reason for hiding this comment

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

👋 Thanks for your contribution @Geod24.

While I agree that pinning to a SHA is not a great user-experience, we need the default starter workflows to follow security best practices. This applies to all 3rd-party actions except for a select few verified organizations.

@andymckay
Copy link
Contributor

The guidance in this repository has changed and that's the basis for this change from @Geod24. However I agree with @joshmgross and the guidance is currently under review internally. I'd like to suggest we merge #763 until we get a definite 👍🏾 on this.

@Geod24 Geod24 force-pushed the dlang-use-version branch 2 times, most recently from f66de58 to 10b07d3 Compare January 4, 2021 16:44
@Geod24
Copy link
Contributor Author

Geod24 commented Jan 4, 2021

This applies to all 3rd-party actions except for a select few verified organizations.

How do we get on that list ? I have admin access to both the dlang and dlang-community organization. dlang is verified on Github.

The guidance in this repository has changed and that's the basis for this change from @Geod24. However I agree with @joshmgross and the guidance is currently under review internally. I'd like to suggest we merge #763 until we get a definite 👍🏾 on this.

If I may... The guidance has been under review for over a year. We were more than happy to transfer ownership to Github and follow whatever practice were decided on. See this issue, open in November 2019.

However, such guideline never materialized. Worse, Github is "moving fast and breaking things" at the expense of users. It took 5 weeks from the deprecation notice to the removal of ::set-env and ::set-path, which were widely used. On the other hand, this PR has been open for 7 weeks.

Merging #763 will fix the issue for now, but I fear this problem will repeat when another issue arise. I'm not asking anyone to shift their focus to dlang, just to have the tools to fix the issues ourselves.

@Geod24
Copy link
Contributor Author

Geod24 commented Feb 2, 2021

On the other hand, this PR has been open for 7 weeks.

Make that 11. Rebased on main.

There are plenty of other actions in this repository using a version
that is not certified by Github. The disclaimer makes this obvious.
The action in question is not simply a user's action, but maintained
by dlang-community, which is overseen by the D Language Foundation.
The recent set of changes to actions means that this workflow is currently
broken, as it was previously using add-path and set-env.
Keeping on upgrading the hash will provide a terrible user experience,
as it is likely to be forgotten, and will also require user-intervention
when another set of change breaks this action, hence why this updates
the hash to a version instead of changing it to the latest hash.
@Geod24 Geod24 force-pushed the dlang-use-version branch from 9484638 to 51ec9ac Compare March 14, 2021 07:13
@Geod24
Copy link
Contributor Author

Geod24 commented Mar 14, 2021

In light of #806, this should be closed.

@Geod24 Geod24 closed this Mar 14, 2021
@Geod24 Geod24 deleted the dlang-use-version branch March 14, 2021 07:21
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