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

Add silent option #526

Merged
merged 28 commits into from Nov 20, 2023
Merged

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Sep 14, 2023

This adds the ability to do a silent release; i.e. a release that will have a placeholder in the changelog and that will keep the release in draft mode.

  • Add silent option
    • It will set a placeholder in the CHANGELOG file
    • It will save the real changelog in the GH release
    • It will not display the changelog entry in the job log
    • It will keep the release as draft (aka private)
    • It will update the changelog file on publication of the release through a new workflow
  • Add workflow to remove the placeholder (I would like to have it execute on release published event to reduce maintenance burden)

One question raised by this proposal is that the audience able to see draft release is broader than the one seeing security advisories

@fcollonval fcollonval added the enhancement New feature or request label Sep 15, 2023
@fcollonval
Copy link
Member Author

code is good for review - but I'm testing it against https://github.com/fcollonval/dummy-again to check it works properly

@fcollonval fcollonval marked this pull request as ready for review September 18, 2023 14:58
@fcollonval
Copy link
Member Author

From the test repo:

@fcollonval
Copy link
Member Author

I updated the documentation

The current documentation is not compatible with the second comment of removing the new parameter from the example workflow. Happy to get suggestion on how to mention the new parameter in the documentation if it is not part of the example worflow.

@Carreau
Copy link

Carreau commented Sep 21, 2023

I have no objections to this, but IIUC to do the release the GH-advisories need to be merged and are public anyway, am I misunderstanding ?

If that's the case I don't think hiding the changelog is going to change things much.

Also one thing it is always save to publish is CVE-number. It is ok to publish them before the CVE/patches are public as it allows users to track wether they might be vulnerable or will need to upgrade once a patch is published.

@Carreau
Copy link

Carreau commented Sep 21, 2023

From the test repo:

Those have (Optional): Review Draft Release: https://github.com/fcollonval/dummy-again/releases/tag/untagged-ead39ca0a2de524e44cc and those gives 404 to me, so it's likely they are private.

@jtpio
Copy link
Member

jtpio commented Sep 21, 2023

I have no objections to this, but IIUC to do the release the GH-advisories need to be merged and are public anyway, am I misunderstanding ?

That was also my initial understanding but thought the process had changed in the meantime.

@fcollonval
Copy link
Member Author

I have no objections to this, but IIUC to do the release the GH-advisories need to be merged and are public anyway, am I misunderstanding ?

This is a good point that I cannot answer. @blink1073 would you mind sharing your experience on that?

@krassowski
Copy link
Contributor

Those have (Optional): Review Draft Release: https://github.com/fcollonval/dummy-again/releases/tag/untagged-ead39ca0a2de524e44cc and those gives 404 to me, so it's likely they are private.

Drafts are only visible to repo/organization maintainers, but I think that logs are visible to all logged in users:

image

@krassowski
Copy link
Contributor

I have no objections to this, but IIUC to do the release the GH-advisories need to be merged and are public anyway, am I misunderstanding ?

From reading GitHub docs, I do not believe that this is the case; they imply that you can merge the pull request from private fork without making the advisory public. Also, you need to provide information on versions affected and version fixed to publish advisory (which you can fake before making the actual release).

I think that ideal the order is:

  1. populate affected versions in advisory
  2. request CVE
  3. merge/backport
  4. release to npm/pypi/conda
  5. populate "patched versions" in advisory
  6. publish advisory
  7. publish changelog (/GitHub release) with link to advisory

This is what the GitHub docs says https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/collaborating-in-a-temporary-private-fork-to-resolve-a-repository-security-vulnerability:

After you merge changes in a security advisory, you can publish the security advisory to alert your community about the security vulnerability in previous versions of your project. For more information, see "Publishing a repository security advisory."

And the fragment which explains that the "patched version" should be populated: https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/publishing-a-repository-security-advisory#about-publishing-a-security-advisory

Warning: Whenever possible, you should always add a fix version to a security advisory prior to publishing the advisory. If you don't, the advisory will be published without a fixed version, and Dependabot will alert your users about the issue, without offering any safe version to update to.

We recommend you take the following steps in these different situations:

  • If a fix version is imminently available, and you are able to, wait to disclose the issue when the fix is ready.
  • If a fix version is in development but not yet available, mention this in the advisory, and edit the advisory later, after publication.
  • If you are not planning to fix the issue, be clear about it in the advisory so that your users don't contact you to ask when a fix will be made. In this case, it is helpful to include steps users can take to mitigate the issue.

@krassowski
Copy link
Contributor

The silent option comes in handy because between merging and backporting there can be a delay of between minutes to hours if something goes wrong (and we had recent experience where GitHub outages would delay the release pipeline by several hours). IMO, whether it is worth a hassle depends on how high severity are the vulnerabilities.

@blink1073
Copy link
Contributor

I agree with @krassowski's assessment

@blink1073
Copy link
Contributor

See also #333

@fcollonval
Copy link
Member Author

18471a4 hide the new changelog entry in the job log if the silent option is checked.

@fcollonval
Copy link
Member Author

@jtpio @krassowski I would like to move forward with this one. There is a hanging question about the example workflow. What are your opinions on that?

@fcollonval
Copy link
Member Author

Hey @blink1073 would you mind reviewing this PR?

@blink1073
Copy link
Contributor

Yep, I reviewed half the files today, will finish tomorrow.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@fcollonval
Copy link
Member Author

Thanks @blink1073 for the review

@fcollonval fcollonval merged commit d77acfd into jupyter-server:main Nov 20, 2023
19 checks passed
@fcollonval fcollonval deleted the enh/allow-silent-release branch November 20, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants