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.

Sign up for GitHub

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

Jump to bottom

Fix package.json version when building npm packages #179

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Fix package.json version when building npm packages #179

merged 1 commit into from
Sep 25, 2023

Conversation

guicaulada
Copy link
Contributor

@guicaulada guicaulada commented Sep 25, 2023 •
edited
Loading

After generating a package with: dagger run go run ./cmd package --version 10.2.0-pre.ab12c34d

Before this change:

$ cat grafana_10.2.0-pre.ab12c34d_maYTJyZOqjCz_linux_amd64/npm-artifacts/@grafana-data-10.2.0-pre.ab12c34d/package.json | jq .version
"10.2.0-pre"

After this change:

$ cat grafana_10.2.0-pre.ab12c34d_SxpFaeTMhkVy_linux_amd64/npm-artifacts/@grafana-data-10.2.0-pre.ab12c34d/package.json | jq .version
"10.2.0-pre.ab12c34d"

@guicaulada guicaulada requested a review from a team September 25, 2023 14:56
kminehart
kminehart requested changes Sep 25, 2023
View reviewed changes
containers/npm_packages.go
c = c.WithExec([]string{"mkdir", "npm-packages"}).
WithExec([]string{"yarn", "install", "--immutable"}).
WithExec([]string{"yarn", "run", "packages:build"}).
// TODO: We should probably start reusing the yarn pnp map if we can figure that out isntead of rerunning yarn install everywhere.
// TODO: We should probably start reusing the yarn pnp map if we can figure that out instead of rerunning yarn install everywhere.
WithExec([]string{"yarn", "run", "lerna", "version", ersion, "--exact", "--no-git-tag-version", "--no-push", "--force-publish", "-y"}).
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to do a yarn install to update the lockfile at this point I believe, because if you don't, then later steps that do yarn install --immutable will fail

Copy link
Contributor Author

@guicaulada guicaulada Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there are any later steps that do yarn install --immutable, should we run yarn install --mode=update-lockfile after updating the version anyway, just to be sure?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmmmmm. Since it's containerized and not using a shared volume I actually don't know if doing that would update it for everything afterwards without exporting that file so maybe for now we should just leave it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if it becomes a problem later then i guess we can resolve it at that point

dsotirakis
dsotirakis reviewed Sep 25, 2023
View reviewed changes
containers/build_frontend.go
c := NodeContainer(d, NodeImage(nodeVersion), platform).
WithDirectory("/src", src).
WithWorkdir("/src")

c = WithYarnCache(c, opts)

ersion := strings.TrimPrefix(version, "v")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ersion := strings.TrimPrefix(version, "v")
version := strings.TrimPrefix(version, "v")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's intentional because it's "version" without the "v" ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

lol, maybe trimmedVersion is better 😂

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that this is not a big deal but I actually think ersion is pretty descriptive. It does look like a typo but it more concisely describes how the version is trimmed. Essentially, it is the exact same as version just with the v removed. Plus we have been using it in a lot of different places so it's probably good to keep it here for consistency.

containers/build_frontend.go
// Get the node version from the 'src' directories '.nvmrc' file.
public := c.
WithExec([]string{"yarn", "install", "--immutable"}).
WithExec([]string{"npm", "version", ersion, "--no-git-tag-version"}).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WithExec([]string{"npm", "version", ersion, "--no-git-tag-version"}).
WithExec([]string{"npm", "version", version, "--no-git-tag-version"}).

containers/npm_packages.go
@@ -14,10 +15,13 @@ func NPMPackages(d *dagger.Client, platform dagger.Platform, src *dagger.Directo

c = WithYarnCache(c, opts)

ersion := strings.TrimPrefix(version, "v")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ersion := strings.TrimPrefix(version, "v")
version := strings.TrimPrefix(version, "v")

containers/npm_packages.go
c = c.WithExec([]string{"mkdir", "npm-packages"}).
WithExec([]string{"yarn", "install", "--immutable"}).
WithExec([]string{"yarn", "run", "packages:build"}).
// TODO: We should probably start reusing the yarn pnp map if we can figure that out isntead of rerunning yarn install everywhere.
// TODO: We should probably start reusing the yarn pnp map if we can figure that out instead of rerunning yarn install everywhere.
WithExec([]string{"yarn", "run", "lerna", "version", ersion, "--exact", "--no-git-tag-version", "--no-push", "--force-publish", "-y"}).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WithExec([]string{"yarn", "run", "lerna", "version", ersion, "--exact", "--no-git-tag-version", "--no-push", "--force-publish", "-y"}).
WithExec([]string{"yarn", "run", "lerna", "version", version, "--exact", "--no-git-tag-version", "--no-push", "--force-publish", "-y"}).

@dsotirakis
Copy link
Contributor

also, unsure what's going to happen with the pre suffix in this case too, @kminehart

@guicaulada
Copy link
Contributor Author

guicaulada commented Sep 25, 2023 •
edited
Loading

Added more information on the PR, the -pre suffix is mantained if provided on the --version flag.

@guicaulada
Copy link
Contributor Author

ersion is version without the v, hence the variable name 😅

@kminehart
Copy link
Collaborator

I think when this is merged we can just remove that update_package_json step in .drone.yml completely.

we don't want the pre suffix 😂 but this changes those package.json versions depending on what is given in the --version arg right so we just need to make sure we're passing in the right version

@guicaulada guicaulada merged commit 1a9beec into main Sep 25, 2023
@guicaulada guicaulada deleted the fix-npm-version branch September 25, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kminehart kminehart kminehart approved these changes

@dsotirakis dsotirakis Awaiting requested review from dsotirakis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guicaulada @dsotirakis @kminehart