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

DCA11Y-1145: Support standard Node version files #3

Merged

Conversation

atl-mk
Copy link
Collaborator

@atl-mk atl-mk commented Jul 24, 2024

The documentation changes should be self-sufficient.

The driving force is friction internal to Atlassian.

@flipatlas flipatlas self-requested a review July 24, 2024 12:21
Copy link
Collaborator

@flipatlas flipatlas left a comment

Choose a reason for hiding this comment

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

lgtm

@gt-lassian gt-lassian self-requested a review July 24, 2024 12:55
Copy link
Collaborator

@rafalsatl rafalsatl left a comment

Choose a reason for hiding this comment

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

Great improvement overall. I left a few smaller comments for your consideration.

@atl-mk atl-mk force-pushed the issue/master/DCA11Y-1145-support-standard-node-version-files branch from c645409 to 9429520 Compare September 20, 2024 17:17
@atl-mk atl-mk force-pushed the issue/master/DCA11Y-1145-support-standard-node-version-files branch 4 times, most recently from cfbc71b to c0fc4aa Compare September 20, 2024 17:58
@atl-mk atl-mk force-pushed the issue/master/DCA11Y-1145-support-standard-node-version-files branch from c0fc4aa to 13eb4d1 Compare September 20, 2024 19:06
Copy link
Collaborator

@flipatlas flipatlas left a comment

Choose a reason for hiding this comment

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

LGTM

frontend-maven-plugin/.nvmrc Show resolved Hide resolved
if (!nodeVersions.get().isPresent()) {
synchronized (NodeVersionHelper.class) { // avoiding racing sending multiple requests
try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
HttpGet getNodeVersionIndex = new HttpGet("https://nodejs.org/dist/index.json");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be based on nodeDownloadRoot property maybe so that we can override node registry?

Copy link
Collaborator

@flipatlas flipatlas left a comment

Choose a reason for hiding this comment

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

lgtm

@flipatlas
Copy link
Collaborator

Topics to discuss raised by @rafalsatl during QA demo:

  1. Should we warn (or even error to ensure reproducible builds) when only major version specified and might resolve to different values if new versions are released?
  2. Could we check for version file, only in the project? Prevent going up the tree looking for version file, outside the project root.

@flipatlas flipatlas merged commit 3a101cc into master Sep 30, 2024
7 checks passed
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.

4 participants