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

For linting and testing auto update the node version to "lts/*" #10323

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

martinfrances107
Copy link
Contributor

This deprecation notice.

version-or-publish
The following actions uses Node.js version which is deprecated and will be forced to run on node20: actions/setup-node@v3. For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/

seen in this test run.

https://github.com/martinfrances107/tauri/actions/runs/9995204078

@martinfrances107
Copy link
Contributor Author

I should explain the second commit.

When I reviewed my own fix... I saw that it should be universally applied to many actions.

Copy link
Member

@jbolda jbolda left a comment

Choose a reason for hiding this comment

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

Appreciate the effort!

with:
node-version: 18
node-version: 20
Copy link
Member

Choose a reason for hiding this comment

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

Please adjust this to node@18. We would like to run the tests on the oldest LTS/maintenance release. That being said, if there is an alias for it, it would be nice to not have to change it.

with:
node-version: 18
node-version: 20
Copy link
Member

Choose a reason for hiding this comment

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

Please adjust this to node@18. We would like to run the tests on the oldest LTS/maintenance release. That being said, if there is an alias for it, it would be nice to not have to change it.

with:
node-version: '18'
node-version: '20'
Copy link
Member

Choose a reason for hiding this comment

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

We could probably set this to lts/* to avoid having to update it again in the future.

@martinfrances107
Copy link
Contributor Author

martinfrances107 commented Jul 20, 2024

Opps so I went stomping arround in clodhoopers over a pre-existing well thought out set of policies. ( thanks for being so polite with the bull in the china shop :) )

... My bad.

For reference I went looking for the node policy, I will quote it at the end. The LTS is changed annually

For our discussion the options from actions/setup-node@v3 that I want to use seem to be "latest" "current" or "lts/*" as appropiate

For this change there is a negative aspect..

When debugging/comparing test between passes and failures... a developer might grimace to discover that the node version is not static from run to run.

But given that policy this will be a "once yearly event"

I just wanted that converstion to be stated in public.

Anyway I will fix all my errors over the weekend.


https://nodejs.org/en/about/previous-releases

Major Node.js versions enter Current release status for six months, which gives library authors time to add support for them. After six months, odd-numbered releases (9, 11, etc.) become unsupported, and even-numbered releases (10, 12, etc.) move to Active LTS status and are ready for general use. LTS release status is "long-term support", which typically guarantees that critical bugs will be fixed for a total of 30 months. Production applications should only use Active LTS or Maintenance LTS releases.

Copy link
Contributor

github-actions bot commented Jul 22, 2024

Package Changes Through c231650

No changes.

Add a change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

Copy link
Member

@jbolda jbolda left a comment

Choose a reason for hiding this comment

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

@martinfrances107 I apologize if I caused confusion here!

My intent is our tests run with specifically defined node 18. As you mentioned, we want to update this with an intent instead of on the LTS schedule. I believe we share the same opinion here!

Seperate from that, I believe we can lts/* with the lint workflows. I don't believe we will be affected by this version changing on their schedule, and it is convenient. I could be convinced to use 18 everywhere though.

.github/workflows/docker.yml Outdated Show resolved Hide resolved
@martinfrances107
Copy link
Contributor Author

Sorry for the delay here .. this had to wait until the weekend

Ok I think have more faithfully expressed the requirements .. here it is in bullets points.

  • Anything publishing or deployment related
    -- A hard coded number. Ensures no surprise updates. Human will always be involved in change.

  • lint/testing
    -- auto increment with a bias to the conservative "LTS"

  • android... I think my best response is the backout the changes in that regard completely
    and let someone else drive change.

Other more informed android devs will have opions about lastes/LTS and for example wheather those measures
should be ignored because they deviate from the long tail of the install base.

Another thing I see in a few places is a list of supported nodes version --

      matrix:
        node:
          - '16'
          - '18'
          - '20'

to my mind I think we can drop support for 16 now, but I think especially when discussing policy.
micro PRs are better.

PS

A PR is incomming.

jbolda
jbolda previously approved these changes Aug 17, 2024
Copy link
Member

@jbolda jbolda left a comment

Choose a reason for hiding this comment

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

Thanks! Appreciate it!

@martinfrances107
Copy link
Contributor Author

Many of the change have already filtered into the dev branch

Mostly due to last saturdays commit

Author: Lucas Fernandes Nogueira [email protected]
Date: Sat Aug 17 08:21:27 2024 -0300
23a912b

So I have redone this PR (hence the force push) with just small items left.

In fact The only thing that remain is ...

lint/testing
-- auto increment with a bias to the conservative "LTS"

@martinfrances107 martinfrances107 changed the title Chore: Fix deprecation notice in action "version-or-publish" For linting and testing auto update the node version to "lts/*" Aug 17, 2024
@lucasfernog lucasfernog merged commit 6b63c75 into tauri-apps:dev Aug 18, 2024
8 checks passed
@martinfrances107 martinfrances107 deleted the version_or_publish branch August 18, 2024 19:21
@martinfrances107
Copy link
Contributor Author

Thank you

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.

3 participants