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

add deprecation to nodejs 16 #337

Merged
merged 3 commits into from
Mar 25, 2024
Merged

Conversation

Jdubrick
Copy link
Contributor

What does this PR do?:

Summarize the changes. Are any stacks or samples added or updated?

This PR adds the Deprecated tag to stacks with the Nodejs 16 image. It also adds these deprecated stacks to the renovate.json exclusion list and bumps the default version for all changed stacks up by 1 so the default version is not a deprecated stack.

Which issue(s) this PR fixes:

Link to github issue(s)
fixes devfile/api#1364

PR acceptance criteria:

  • Contributing guide
    Have you read the devfile registry contributing guide and followed its instructions?
  • Test automation
    Does this repository's tests pass with your changes?
  • Documentation
    Does any documentation need to be updated with your changes?
  • Check Tools Provider
    Have you tested the changes with existing tools, i.e. Odo, Che, Console? (See devfile registry contributing guide on how to test changes)

How to test changes / Special notes to the reviewer:

@Jdubrick Jdubrick requested a review from a team as a code owner March 19, 2024 16:07
renovate.json Outdated Show resolved Hide resolved
@Jdubrick
Copy link
Contributor Author

/retest

@Jdubrick
Copy link
Contributor Author

/hold

This is going to need to be merged after the nodejs issue is fixed devfile/api#1484

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

I think based on what we have discussed for devfile/api#1482 for the stacks we update the default version we can make the new default their latest version. WDYT?

@Jdubrick
Copy link
Contributor Author

I think based on what we have discussed for devfile/api#1482 for the stacks we update the default version we can make the new default their latest version. WDYT?

That's okay with me, I know I just bumped them all up 1 but can update it to bump them all to latest. I'm going to also mark the issue as blocked since it isn't going to pass PR checks with the nodejs icon issue

cc @thepetk

@Jdubrick
Copy link
Contributor Author

/retest

@michael-valdron
Copy link
Member

michael-valdron commented Mar 20, 2024

Just need #338 merged after hearing back from the stack owner before the builds will resume working again

FYI @Jdubrick

@Jdubrick
Copy link
Contributor Author

/retest

@michael-valdron
Copy link
Member

/unhold

#338 should have unblocked this PR.

@Jdubrick
Copy link
Contributor Author

Will re run tests in a little bit, seems like the issue is still occuring but I don't know maybe it hasn't updated yet to CI. https://github.com/devfile/registry/actions/runs/8348517850/job/22941030162?pr=337

@michael-valdron
Copy link
Member

Will re run tests in a little bit, seems like the issue is still occuring but I don't know maybe it hasn't updated yet to CI. https://github.com/devfile/registry/actions/runs/8348517850/job/22941030162?pr=337

Most likely, ci/prow/v4.13-images is passing now.

@michael-valdron
Copy link
Member

@lholmquist Need your quick review on the changes done to the nodejs stack.

FYI @Jdubrick

@michael-valdron michael-valdron removed the request for review from BethGriggs March 21, 2024 16:28
Copy link

openshift-ci bot commented Mar 21, 2024

@lholmquist: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

@Jdubrick Just realized you haven't rebased since #338 was merged 🙈

So your version of the code is still got the broken nodejs icon links.

Signed-off-by: Jordan Dubrick <[email protected]>
Signed-off-by: Jordan Dubrick <[email protected]>
@Jdubrick Jdubrick force-pushed the nodejs-deprecation branch from 0b693a3 to 358ac44 Compare March 22, 2024 19:11
@Jdubrick
Copy link
Contributor Author

Jdubrick commented Mar 22, 2024

@Jdubrick Just realized you haven't rebased since #338 was merged 🙈

So your version of the code is still got the broken nodejs icon links.

@michael-valdron i was trying to figure out why the builds were still failing, thank you. Just rebased so hopefully this fixes the issue!

edit: svelte change added a different default version and my rebase didnt notify me of that

@Jdubrick
Copy link
Contributor Author

@michael-valdron looks like the rebase and nodejs changes allowed checks to pass

stacks/nodejs-vue/stack.yaml Show resolved Hide resolved
stacks/nodejs-react/stack.yaml Show resolved Hide resolved
stacks/nodejs-nuxtjs/stack.yaml Show resolved Hide resolved
stacks/nodejs-nextjs/stack.yaml Show resolved Hide resolved
stacks/nodejs-angular/stack.yaml Show resolved Hide resolved
@Jdubrick
Copy link
Contributor Author

@thepetk in response to your reviews the defaults were just bumped up 1 version from the one that was deprecated in this case. This was done before any discussion surrounding if we should keep the default updated to the latest version. wdyt?

@thepetk
Copy link
Contributor

thepetk commented Mar 25, 2024

@thepetk in response to your reviews the defaults were just bumped up 1 version from the one that was deprecated in this case. This was done before any discussion surrounding if we should keep the default updated to the latest version. wdyt?

I'd say in order to avoid back & forth for this PR we can move the default to latest with a separate PR!

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

/lgtm as @michael-valdron requested changes I'd wait for his approval too :)

@openshift-ci openshift-ci bot added the lgtm Looks good to me label Mar 25, 2024
Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Mar 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jdubrick, lholmquist, michael-valdron, thepetk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Jdubrick,michael-valdron,thepetk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Jdubrick Jdubrick merged commit 2675083 into devfile:main Mar 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate stack versions with images using node.js 16
4 participants