Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

GDL-9 publish npm package to registry #206

Open
wants to merge 1 commit into
base: development/7.10
Choose a base branch
from

Conversation

tcarmet
Copy link
Contributor

@tcarmet tcarmet commented Jan 24, 2025

Release workflow that will publish guidelines as @scality/guidelines on both GitHub Packages and Npmjs.org.

I recommend us to spend some time here as this will kind of set the picture for the rest of our packages, the work will be pretty much a copy paste from here.

Publishing on GitHub Packages could arguably be optional, but I believe it helps with some internal discovery things like dependabot or security. Additionally, I believe that privates one will be published into GitHub Packages, so at least all of our packages will be with one registry if it's something we will look into in the future.

The most important one to set it's npmjs.org. because even though we can host public packages under the GitHub umbrella, it still enforces users to login with GitHub to be able to pull them, and I'm afraid this some unnecessary complexity, which may break scripts, or user workflows, etc.

@bert-e
Copy link
Contributor

bert-e commented Jan 24, 2025

Hello tcarmet,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options
name description privileged authored
/after_pull_request Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval Bypass the pull request author's approval
/bypass_build_status Bypass the build and test status
/bypass_commit_size Bypass the check on the size of the changeset TBA
/bypass_incompatible_branch Bypass the check on the source branch prefix
/bypass_jira_check Bypass the Jira issue check
/bypass_peer_approval Bypass the pull request peers' approval
/bypass_leader_approval Bypass the pull request leaders' approval
/approve Instruct Bert-E that the author has approved the pull request. ✍️
/create_pull_requests Allow the creation of integration pull requests.
/create_integration_branches Allow the creation of integration branches.
/no_octopus Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity Change review acceptance criteria from one reviewer at least to all reviewers
/wait Instruct Bert-E not to run until further notice.
Available commands
name description privileged
/help Print Bert-E's manual in the pull request.
/status Print Bert-E's current status in the pull request TBA
/clear Remove all comments from Bert-E from the history TBA
/retry Re-start a fresh build TBA
/build Re-start a fresh build TBA
/force_reset Delete integration branches & pull requests, and restart merge process from the beginning.
/reset Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Jan 24, 2025

Incorrect fix version

The Fix Version/s in issue GDL-9 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 7.10.6

  • 8.2.2

  • 8.3.1

Please check the Fix Version/s of GDL-9, or the target
branch of this pull request.

@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch from 1761cd4 to fb466cd Compare January 24, 2025 22:28
@bert-e
Copy link
Contributor

bert-e commented Jan 24, 2025

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

@francoisferrand
Copy link
Contributor

Before jumping into implementation, can we write a TAD to discuss the various issues and solutions?

There are quite a few questions to investigate, impacts on dev process to evaluate and discuss: like at least

  • should we publish to github packages only (consistent, no ambiguity), npm ("standard", but may require separate login, security risk...), or both (github packages + npm for public repository)
  • should we publish only releases, or also each commit/pr?
  • how do devs "test" the integration of a dependency not yet released? One benefit of pushing is that we compile TS just once, and we will thus remove the hacks we had to do to support typescript from "git" deps: so we need an alternative way to handle this
  • if we push every PR, it will build up: what convention do we use to "tag" the packages unambiguously? Does the registry scale as needed? Do we need to implement some cleanup workflow?
  • what is the setup needed for developers to consume the private packages
  • there are some packages with native parts, which are rebuilt on install: can we prebuilt those? (Likely a secondary problem to address later, but best to remember it and assess if we can indeed handle it later)
  • and there may be more questions each dev will have, based on their experience...

@tcarmet tcarmet requested a review from dvasilas January 27, 2025 18:06
@tcarmet
Copy link
Contributor Author

tcarmet commented Jan 27, 2025

Sounds good @francoisferrand, I'll open a PR in citadel.

@BourgoisMickael
Copy link

BourgoisMickael commented Jan 27, 2025

Sounds good @francoisferrand, I'll open a PR in citadel.

@tcarmet Rather than citadel that is for component design we write in confluence in object squad the TAD

https://scality.atlassian.net/wiki/spaces/OS/pages/1573289989/Technical+Approach+Documents+TADs

@tcarmet
Copy link
Contributor Author

tcarmet commented Jan 27, 2025

okk understood, thanks @BourgoisMickael

@tcarmet
Copy link
Contributor Author

tcarmet commented Jan 27, 2025

As requested, here's the TAD

cc: @francoisferrand @BourgoisMickael

@tcarmet tcarmet marked this pull request as draft February 11, 2025 17:09
@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch 2 times, most recently from 9c07d80 to 710c2a0 Compare February 14, 2025 10:58
@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch 5 times, most recently from 03dd4a7 to 6ad5454 Compare February 14, 2025 15:52
@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch 2 times, most recently from 83629d8 to 951faba Compare February 18, 2025 21:01
with:
registry-url: https://registry.npmjs.org
- name: Publish to npmjs.org
run: npm publish --provenance
Copy link
Contributor

@francoisferrand francoisferrand Feb 19, 2025

Choose a reason for hiding this comment

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

what happens if the push to npmjs fails, i.e. how can be "finisih" the release?

  • there is a "tag" check, which prevents generating the tag if already done : OK since the tag (and release) is created last
  • when publish to Github packages fails, nothing was actually changed : OK
  • when publish to npmjs fails, github package has already been published:
    • re-running may fail (?) as we already pushed to github?
    • if we re-run the release, we may have a different commit on the "new" release.... and could end-up with an incorrect build on github?
    • in the mean-time, the release is not present, but the package is already in github and may be consummed/upgraded unexpectedly... should we remove it? or make the tag/release first (but this adds complexity to re-run)?
    • as the tag is not created, bert-e (when used) may also mark more jira tickets as "fixed" in this release... this is not a new issue, but agravated if the release takes longer...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You wanna change the order, this way if it fails on npm it didn't published on GitHub packages?

Also let's try not to overthink this too much, let's get some field experience first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to change the order, I want to merge something where we have a path, in case of failure : because such failures do and will happen, and typically when it happens it is at the worst of times...

So it is really not overthinking, but actually just thinking it through, to try and have some way to finish the release anyway when we need it.

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 see, ran a couple of test and confirm that we will face some failure if for example we want to re-publish a package to GitHub (I guess the same will happen on npm):

npm error code E409
npm error 409 Conflict - PUT https://npm.pkg.github.com/@scality%2feslint-config - Cannot publish over existing version
npm error A complete log of this run can be found in: /home/codespace/.npm/_logs/2025-02-21T21_38_24_404Z-debug-0.log

Now, my main concern here, is not towards what you want to apply, it's mostly that we are adding too much code/logic into something that as far as we established will be copy pasted.

So if we are duplicating code, let's keep it "dumb" even if the feature are limited. But if we want to add more checks and all, I rather we develop an action and reuse that code, otherwise it will be a pain to maintain across 20 different repos.

Let me know which path you prefer, either are fine by me I don't mind developing an action for it.

@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch from 951faba to 1b3c5a3 Compare February 19, 2025 18:27
@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch from 1b3c5a3 to 240ff79 Compare February 19, 2025 18:35
@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch from 69e8627 to 6a8f240 Compare February 20, 2025 22:06
@tcarmet tcarmet force-pushed the feature/GDL-9-publish-npm-registry branch from 6a8f240 to 0d4a7a2 Compare February 21, 2025 21:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants