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(config): user-agent properly shows ci #3754

Merged
merged 1 commit into from Sep 14, 2021
Merged

Conversation

wraithgar
Copy link
Member

The way we were flattening user-agent back into itself meant that any
values that were dependent on other config items would never be seen,
since we have to re-flatten the item for each one it depends on.

We also weren't re-flattening the user-agent when setting workspaces or
workspace, which were things that affected the final result.

This does change the main config value of user-agent but not the
flattened one. We are not using the main config value anywhere (which
is correct).

@wraithgar wraithgar requested a review from a team as a code owner September 14, 2021 14:32
@darcyclarke
Copy link
Contributor

@wraithgar can we also write an E2E/smoke test that ensures this is sent to the registry properly when running a command w/ -w / -ws?

@wraithgar wraithgar force-pushed the gar/user-agent-ci branch 2 times, most recently from 54087b0 to 0de45b8 Compare September 14, 2021 15:33
@isaacs
Copy link
Contributor

isaacs commented Sep 14, 2021

As long as we still send a valid userAgent option in the flatOptions to all our modules, I don't think there's too much to discuss here. The only user-visible change is that process.env.npm_config_user_agent will no longer be the flattened version, correct?

@wraithgar
Copy link
Member Author

Good point, the env one should ... probably be flattened?

@darcyclarke darcyclarke added semver:patch semver patch level for changes Release 7.x work is associated with a specific npm 7 release labels Sep 14, 2021
@nlf
Copy link
Contributor

nlf commented Sep 14, 2021

As long as we still send a valid userAgent option in the flatOptions to all our modules, I don't think there's too much to discuss here. The only user-visible change is that process.env.npm_config_user_agent will no longer be the flattened version, correct?

i verified with node . run dumpconf that the environment variable is the flattened version, so we're good there

@nlf
Copy link
Contributor

nlf commented Sep 14, 2021

@wraithgar can we also write an E2E/smoke test that ensures this is sent to the registry properly when running a command w/ -w / -ws?

added a smoke test, it's a bit hacky but it gets the job done!

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this looks very much correct. smoke test was added too so 👍

The way we were flattening user-agent back into itself meant that any
values that were dependent on other config items would never be seen,
since we have to re-flatten the item for each one it depends on.

We also weren't re-flattening the user-agent when setting workspaces or
workspace, which were things that affected the final result.

This does change the main config value of `user-agent` but not the
flattened one.  We are not using the main config value anywhere (which
is correct).

PR-URL: #3754
Credit: @wraithgar
Close: #3754
Reviewed-by: @nlf
@wraithgar wraithgar merged commit b4aac34 into release-next Sep 14, 2021
@wraithgar wraithgar deleted the gar/user-agent-ci branch September 14, 2021 17:17
@fritzy fritzy mentioned this pull request Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants