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

documentation: document libsecret dependency #9807

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Conversation

vince-fugnitto
Copy link
Member

What it does

The pull-request updates the documentation for keytar (a native node dependency) which requires libsecret.

  • update the prerequisites including anchor links.
  • update the changelog for the update.

How to test

  • verify the content of the changes.

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

The commit updates the documentation for `keytar` (a native node
dependency) which requires `libsecret`.

- update the `prerequisites` including anchor links.
- update the `changelog` for the update.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto vince-fugnitto added the documentation issues related to documentation label Jul 30, 2021
@marcdumais-work
Copy link
Contributor

Two things:
1 - I feel a bit strange about updating the CHANGELOG for an already released version
2 - So far, I think we have mostly documented API breakage in CHANGELOG, I think?

Maybe this is the opportunity to create a "migration-guide.md" or such, in which we would document things that "break", beyond APIs?

cc: @tsmaeder , @JonasHelming

@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 2, 2021

@marcdumais the change log should still call out breaking changes: the migration guide tells the user how, if it's too complicated to describe in a compact change log entry.

Fixing the change log for the 1.16.0 release is the right thing to do. The question is whether we want to release 1.16.1 because of this documentation-only change. I think yes, unless the effort is out of proportion to the benefit. @vince-fugnitto what's your opinion?

@vince-fugnitto
Copy link
Member Author

The question is whether we want to release 1.16.1 because of this documentation-only change. I think yes, unless the effort is out of proportion to the benefit. @vince-fugnitto what's your opinion?

@tsmaeder it would likely be a patch release of 1.15.1 since that is when libsecret was added at the incorrect version range in terms of compatibility. Given that the release would be functionally equivalent to 1.15.0 (since its only a changelog update) I was thinking that it would not be necessary to provide a patch release.

@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 3, 2021

@vince-fugnitto I'm just worried that when someone checks out the 1.15 release, they are not seeing the changelog entries. We should merge this PR anyway and we can discuss our policy on backporting in the community call maybe?

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto I'm just worried that when someone checks out the 1.15 release, they are not seeing the changelog entries. We should merge this PR anyway and we can discuss our policy on backporting in the community call maybe?

@tsmaeder I'm not sure anyone does a checkout of a release as such, or even uses the github releases (ex: https://github.com/eclipse-theia/theia/releases/tag/v1.15.0). If necessary I can perform a patch release, I just did not know if the overhead would be worth it for a documentation update which highlights potential breakage due to keytar.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

LGTM as well. I agree with Vince there, I don't see any benefits in a patch release.

@benoitf
Copy link
Contributor

benoitf commented Aug 3, 2021

FYI you may add that downgrading keytar implies that there is no longer prebuilt binaries for Alpine that are available. (they're only there since keytar 7.6.0

@vince-fugnitto
Copy link
Member Author

FYI you may add that downgrading keytar implies that there is no longer prebuilt binaries for Alpine that are available. (they're only there since keytar 7.6.0

@benoitf perhaps it should be included as part of #9817, or did you mean adding a note in the 1.16.0 changelog which performed the downgrade?

@benoitf
Copy link
Contributor

benoitf commented Aug 3, 2021

@vince-fugnitto up to you just that it broke in our Alpine image after the downgrade due to the missing prebuilt binary. (so now libsecret-devel is a mandatory package at build time while before only libsecret lib at runtime was a requirement)

@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 3, 2021

@benoitf the migration guide is supposed to be a living document that adopters update if they find issues with migrating to a particular release.

@tsmaeder tsmaeder merged commit cff1eda into master Aug 3, 2021
@github-actions github-actions bot added this to the 1.17.0 milestone Aug 3, 2021
@vince-fugnitto vince-fugnitto deleted the vf/libsecret-docs branch August 3, 2021 13:34
@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 3, 2021

The idea for the migration guide came out of the observation that we all had to solve similar problems with the webpack upgrade. If the first adopter had written down what the migration procedure is, many people could have saved time.

dna2github pushed a commit to dna2fork/theia that referenced this pull request Aug 25, 2021
The commit updates the documentation for `keytar` (a native node
dependency) which requires `libsecret`.

- update the `prerequisites` including anchor links.
- update the `changelog` for the update.

Signed-off-by: vince-fugnitto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation issues related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants