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

Enable GitHub release #56

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Conversation

ljubon
Copy link
Contributor

@ljubon ljubon commented Jan 26, 2024

The goal of this PR is to add a release job with a target for GitHub
The Nuget Release job is already present, the same package/artifacts are used to publish it to the GitHub release

https://github.com/G-Research/oss-portfolio-maturity/issues/332

@ljubon ljubon requested review from a team as code owners January 26, 2024 14:21
@Smaug123
Copy link
Contributor

Does anyone actually get any value from GitHub releases for NuGet packages? I claim the answer is "no"; is there any evidence that they do?

@pavlovic-ivan
Copy link
Contributor

Thank you @Smaug123 for your perspective on GitHub releases for NuGet packages. While it's true that the value of this practice may vary depending on individual needs and project contexts, many teams find it beneficial because of transparency, documentation (Release notes in GitHub are central and accessible) and community engagement. However, if you strongly disagree, we can close this PR as not needed. Please let us know.

Best regards

@nojaf
Copy link
Contributor

nojaf commented Jan 26, 2024

In other projects I have seen that some people subscribe to releases:
image

If you cannot use something like dependabot for example...

@pavlovic-ivan
Copy link
Contributor

pavlovic-ivan commented Jan 26, 2024

Yes, and i've also seen other projects like winsw that are strictly C# (meaning not other languages used in the project), that publish to Github releases. But like i've said, if in general we don't want this, that's ok. I would just like to hear/read a definitive decision

Copy link
Contributor

@Smaug123 Smaug123 left a comment

Choose a reason for hiding this comment

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

I guess it does no harm

needs: [all-required-checks-complete]
environment: release
permissions:
contents: write
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a bit sad that GitHub's perms model isn't more finely grained here. This gives permission to update the commit hash of the main branch, for example.

Copy link
Contributor Author

@ljubon ljubon Jan 29, 2024

Choose a reason for hiding this comment

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

Hi @Smaug123 if it counts - this is giving write permission only to this specific job, it's not possible to leverage this permission outside of this job
This is required as we are modifying/publishing releases which are part of this repo

Choose a reason for hiding this comment

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

This job executes external code that you do not have control of: actions/download-artifact@v4 and ncipollo/release-action@v1. Attacks on that code will be able to make arbitrary use of the write permission.

It is good practice to pin actions to a specific commit (not a tag like v1 that can change over time) plus a thorough review of that commit's code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with that, but it conflicts with dependabot or any other dependency management that might want to update the action to the latest one due to security reasons or new features
It's up to the maintainer to decide whether he should be responsible for checking latest versions of the source code for any actions in this job, or having that in mind when merging PRs from tools like dependabot

Copy link

@EnricoMi EnricoMi Jan 29, 2024

Choose a reason for hiding this comment

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

Dependabot supports those commit versions as well, it keeps suggesting latest versions (here commits). Example: EnricoMi/publish-unit-test-result-action#473

Copy link
Contributor

@pavlovic-ivan pavlovic-ivan Jan 29, 2024

Choose a reason for hiding this comment

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

We haven't reached a point where we should distrust the tags yet. If we did, a problem would be a lot bigger than what we are discussing here, and there would be hundreds of PRs opened in this org alone to mitigate the issue. I do agree with the practice, but not so far to block PR from merging (i am aware it's approved). I will add though that when it comes to github actions (official and 3rd party), that we plan to have proxy actions in place of those, where we would do exactly what is mentioned (pin to a commit, and update to the next if review of the changes allows it). That will generate a lot of PRs over time as expected. I hope this gives some level of serenity

Copy link
Contributor

Choose a reason for hiding this comment

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

Having just checked what that action does, a single line of curl would be simpler and easier to review :P

Copy link
Contributor

Choose a reason for hiding this comment

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

True that 😅

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

I'm approving because Patrick did.
This looks good, but I lack a strong opinion on this.

@pavlovic-ivan pavlovic-ivan merged commit c7255bc into G-Research:main Jan 30, 2024
22 checks passed
@Smaug123
Copy link
Contributor

Please revert this; it's broken. Life's too short to work out why, since nobody actually requested this.

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.

5 participants