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

[ui] Namespaced and file-specified deps for consul-ui #21378

Merged
merged 5 commits into from
Jul 8, 2024
Merged

[ui] Namespaced and file-specified deps for consul-ui #21378

merged 5 commits into from
Jul 8, 2024

Conversation

philrenaud
Copy link
Collaborator

Description

This resolves an ambiguous dependency resolution in consul-ui in the event that a developer attempted to yarn install or npm install from a non-workspace-root directory. This handles it in two ways:

  • Namespaces with a @hashicorp/ prefix, reducing the risk of a hypothetical public registry squat situation should local packages ever stop being relative
  • Explicitly locally dependency-links packages

Something to note with this is that the packages become a little more bound together than before; for example, it would be more difficult to maintain multiple valid versions of a sub-package here and let consul-ui differentiate between them. We could expand this into submodules some day, but not today.

@philrenaud philrenaud self-assigned this Jun 27, 2024
@github-actions github-actions bot added the theme/ui Anything related to the UI label Jun 27, 2024
@dduzgun-security
Copy link
Collaborator

Awesome work @philrenaud, thanks a lot for doing that this quickly 🙌
One small comment about consul-ui, should we bump the version to 2.2.1?

@dduzgun-security dduzgun-security added the pr/needs-rebase PR needs to be rebased before merging label Jun 27, 2024
@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch from 4007e05 to e18b631 Compare July 2, 2024 19:48
@philrenaud
Copy link
Collaborator Author

Note: in order to resolve a non-standard license in a dependency-of-dependency-of-dependency, in 36fef17e6cdc9dbb088de1bbad9bc4ed44eee6ee I've done two things:

  • pinned tailwindcss to a specific version that no longer imports the dependency chain that would ultimately include a non-standard license
  • made a config file at .yarnrc to specify that package resolution should come from registry.npmjs.org rather than registry.yarnpkg.com. This was a precedent set in Upgrade Consul UI to Node 18 (#19252) #19362 (see yarn lock for all the domain changes) likely due to an environment variable for the upgrader at the time. This change enshrines it.

@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch from ee4cc11 to 7277670 Compare July 6, 2024 14:07
@philrenaud philrenaud added backport/1.19 This release series is longer active on CE, use backport/ent/1.19 backport/all Apply backports for all active releases per .release/versions.hcl and removed pr/needs-rebase PR needs to be rebased before merging backport/1.19 This release series is longer active on CE, use backport/ent/1.19 labels Jul 6, 2024
@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch 2 times, most recently from a2e4715 to 79d8b85 Compare July 8, 2024 19:53
@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch from 79d8b85 to 8d13103 Compare July 8, 2024 20:15
dduzgun-security
dduzgun-security approved these changes Jul 8, 2024
View reviewed changes
Copy link
Collaborator

@dduzgun-security dduzgun-security left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Huge thanks for fixing this and for all the details you have provided for the resolution, it's awesome 👍
We can merge once tests are passing

@philrenaud philrenaud merged commit dce6241 into main Jul 8, 2024
70 checks passed
@philrenaud philrenaud deleted the NET-10157-consul-ui-devdep-specificity branch July 8, 2024 20:36
@hc-github-team-consul-core hc-github-team-consul-core added backport/1.19 This release series is longer active on CE, use backport/ent/1.19 backport/ent/1.17 This release series is longer active on CE or Ent labels Jul 8, 2024
@hc-github-team-consul-core hc-github-team-consul-core added backport/ent/1.15 Changes are backported to 1.15 ent backport/ent/1.18 Changes are backported to 1.18 ent labels Jul 8, 2024
@hc-github-team-consul-core hc-github-team-consul-core mentioned this pull request Jul 8, 2024
Merged
jmurret pushed a commit that referenced this pull request Jul 8, 2024
@philrenaud @jmurret
[ui] File-specified deps for consul-ui (#21378)
ded0cce 
* Namespaced and file-specified deps

* Pinning to a specific version of tailwind and setting config for js packages to come from npmjs

* Pin glob instead of reverting tailwind or any other (grand)parent dependency

* ember-cli-build fixed path resolution for now-namespaced submodules

* Dropping the namespace prefix and relying on relative pathing
@adamcstephens adamcstephens mentioned this pull request Jul 19, 2024
Merged
13 tasks
@dhiaayachi dhiaayachi mentioned this pull request Sep 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dduzgun-security dduzgun-security dduzgun-security approved these changes

@rboyer rboyer Awaiting requested review from rboyer

@finnigja finnigja Awaiting requested review from finnigja

@eastebry eastebry Awaiting requested review from eastebry

Assignees

@philrenaud philrenaud

Labels
backport/all Apply backports for all active releases per .release/versions.hcl backport/ent/1.15 Changes are backported to 1.15 ent backport/ent/1.17 This release series is longer active on CE or Ent backport/ent/1.18 Changes are backported to 1.18 ent backport/1.19 This release series is longer active on CE, use backport/ent/1.19 theme/ui Anything related to the UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philrenaud @dduzgun-security @hc-github-team-consul-core