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

Update scope for npm dependencies #11615

Merged
merged 2 commits into from Aug 22, 2022
Merged

Conversation

alexvy86
Copy link
Contributor

Description

As part of recent changes to some of our internal infrastructure I updated the scope for some NPM dependencies in the @ff-internal/aria-logger package. Pipelines are currently installing the latest version of aria-logger but setting up authentication for the older scope for the dependencies.

This PR updates the scope/feed used in the pipelines and pins the installed version of aria-logger so that future changes to it aren't a (potential) immediate break in the pipelines.

Reviewer Guidance

Pinning the version of aria-logger that we install in the pipelines means one more change we need to make there in order for any improvements made to aria-logger to be reflected, but it makes them a bit more robust. Given the low frequency of changes to aria-logger, I think the trade-off is acceptable.

@alexvy86 alexvy86 requested a review from a team as a code owner August 22, 2022 18:34
@github-actions github-actions bot added area: build Build related issues base: main PRs targeted against main branch labels Aug 22, 2022
@scarlettjlee
Copy link
Contributor

How might we know in the future that we need to update the version of aria-logger?

@alexvy86
Copy link
Contributor Author

How might we know in the future that we need to update the version of aria-logger?

Since (AFAIK) it is only used in the pipelines, any improvements to it will probably come from a need there. I'd imagine the person who makes the improvements will know that this pinned installation step exists and will update it together with their change, or will investigate why their improvement isn't being reflected in the pipeline and eventually run into the pinned version and update it.

@alexvy86 alexvy86 merged commit 034eafc into microsoft:main Aug 22, 2022
@alexvy86 alexvy86 deleted the fix-test-pipelines branch August 22, 2022 19:03
@github-actions
Copy link
Contributor

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

@@ -186,7 +186,7 @@ jobs:
inputs:
command: 'custom'
workingDir: ${{ parameters.testWorkspace }}
customCommand: 'install $(Initialize.testPackageTgz) @ff-internal/aria-logger'
customCommand: 'install $(Initialize.testPackageTgz) @ff-internal/aria-logger@0.0.11'
Copy link
Member

Choose a reason for hiding this comment

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

@alexvy86 This makes it kind of painful to update the logger - what's the benefit you're going for? Is this necessary to have as part of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not absolutely necessary, and I see the pain point. Above I mentioned how the slower cadence of changes in the logger make me think that it shouldn't be too painful, and the advantage is that the pipelines are protected from immediately breaking if the changes in the logger need additional work on this side (like what happened with my change to the npm scope of the dependencies). I can also see an argument that those kinds of changes in the logger are infrequent enough that it might be better to float the version here and let the sporadic issues arise and get fixed when they do.

Just a matter of how protective we want to be with the pipelines, I suppose. But I'd be fine floating it, I'll do a new PR for that.

Copy link
Member

Choose a reason for hiding this comment

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

My bad for not reading the PR description! I can see it both ways too.

alexvy86 added a commit to alexvy86/FluidFramework that referenced this pull request Aug 25, 2022
alexvy86 added a commit to alexvy86/FluidFramework that referenced this pull request Aug 25, 2022
tylerbutler pushed a commit that referenced this pull request Aug 25, 2022
* Update scope for npm dependencies and pin aria-logger version (#11615)
* Float version when installing aria-logger (#11623)
tyler-cai-microsoft pushed a commit to tyler-cai-microsoft/FluidFramework that referenced this pull request Sep 27, 2022
tyler-cai-microsoft pushed a commit to tyler-cai-microsoft/FluidFramework that referenced this pull request Sep 27, 2022
tyler-cai-microsoft added a commit that referenced this pull request Sep 27, 2022
#12153)

Port this #11615 into
0.59.400x to fix the e2e tests

Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants