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

fix zdeps -version output with dummy @yarnpkg/plugin-github #2349

Merged
merged 3 commits into from
May 9, 2022
Merged

Conversation

nwt
Copy link
Member

@nwt nwt commented May 5, 2022

Both Brimcap and Zed must be built in a cloned Git repository to ensure
that "brimcap -version", "zed -version", and "zq -version" produce
meaningful output, but Yarn's @yarnpkg/plugin-github plugin fetches
tarballs for GitHub repositories instead of cloning them. Add a dummy
plugin so Yarn will clone repositories instead.

Closes #2311.

@nwt nwt requested review from mattnibs and a team May 5, 2022 18:52
@nwt nwt marked this pull request as draft May 5, 2022 18:55
Comment on lines 84 to 87
# The "git@github.com:" prefix causes Yarn to clone the repository
# rather than fetch a tarball. This ensures "zed -version" and
# "zq -version" produce meaningful output.
- run: yarn add zed@git@github.com:brimdata/zed#${{ steps.zed_pr.outputs.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about relying on ssh git, as it feels like github is slowly trying to get rid of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot that SSH authentication won't work in CI without some fiddling, so I went in a different direction and disabled the @yarnpkg/plugin-github plugin.

(I don't think SSH is on the way out, though. Perhaps you're thinking of the git:// protocol, which they've already disabled?)

@nwt nwt force-pushed the zdeps-version branch from 6716c28 to 72ad820 Compare May 5, 2022 21:09
Both Brimcap and Zed must be built in a cloned Git repository to ensure
that "brimcap -version", "zed -version", and "zq -version" produce
meaningful output, but Yarn's @yarnpkg/plugin-github plugin fetches
tarballs for GitHub respotories instead of cloning them.  Add a dummy
plugin so Yarn will clone repositories instead.
@nwt nwt force-pushed the zdeps-version branch from 72ad820 to 6e5d67a Compare May 5, 2022 21:18
@nwt nwt changed the title fix zdeps -version output @nwt fix zdeps -version output with dummy @yarnpkg/plugin-github May 5, 2022
@nwt nwt changed the title @nwt fix zdeps -version output with dummy @yarnpkg/plugin-github fix zdeps -version output with dummy @yarnpkg/plugin-github May 5, 2022
@nwt
Copy link
Member Author

nwt commented May 5, 2022

Because CI enables Yarn caching, this effect of this change won't be visible until the next bump of Brimcap and Zed.

You can see this working with the Yarn cache disabled at https://github.com/brimdata/brim/runs/6313928920?check_suite_focus=true#step:6:769.

@nwt nwt marked this pull request as ready for review May 5, 2022 22:03
Copy link
Member

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

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

Easy enough! I don't think this is necessary now, but would there be a way to only do this for the zed/brimcap repos, and let the other repos use the tarball way?

@nwt
Copy link
Member Author

nwt commented May 9, 2022

Easy enough! I don't think this is necessary now, but would there be a way to only do this for the zed/brimcap repos, and let the other repos use the tarball way?

That's certainly possible, though at present Brimcap and Zed are the only two GitHub repository dependencies in package.json, which means they're the only two dependencies affected by the dummy plugin.

@nwt nwt merged commit f65aae6 into main May 9, 2022
@nwt nwt deleted the zdeps-version branch May 9, 2022 17:48
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.

Bundled brimcap does not return a version string (regression at 0673659)
3 participants