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

[new-ui] Bump angular version to 12 #1712

Merged
merged 27 commits into from
Jan 13, 2022

Conversation

seong7
Copy link
Contributor

@seong7 seong7 commented Oct 13, 2021

What this PR does / why we need it:
Updates Angular version from 8 to 12 (latest).
Details are described in the issue #1590

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1590

Checklist:

  • Docs included if any changes are user facing

@aws-kf-ci-bot
Copy link
Contributor

Hi @seong7. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: seong7
To complete the pull request process, please assign kimwnasptd after the PR has been reviewed.
You can assign the PR to them by writing /assign @kimwnasptd in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@kimwnasptd
Copy link
Member

@seong7 could you instead use either node v12 or node v14 and update the package-lock.json? Doing an npm i should do the trick, after you used on of the above versions.

I'm OK with going with node v14 as well, but in that case please add a commit to:

  1. Update the README of the frontend to mention the new version of Node, and lets also mention the npm version https://github.com/kubeflow/katib/blob/master/pkg/new-ui/v1beta1/README.md#requirements
  2. Update the dockerfile of the web app to use that version of node in https://github.com/kubeflow/katib/blob/master/cmd/new-ui/v1beta1/Dockerfile#L12 and in https://github.com/kubeflow/katib/blob/master/cmd/new-ui/v1beta1/Dockerfile#L24

@andreyvelich
Copy link
Member

Thank you for the review @kimwnasptd.
@seong7 What do you think about this PR ? Are we going to finish it before Kubeflow 1.5 release ?

@seong7
Copy link
Contributor Author

seong7 commented Dec 1, 2021

@kimwnasptd
Since this PR is entitled to bump Angular version not Node and npm, I think we better stick with node v12 and npm 6.
For Node and npm, we will need separated issue to work only if we need.
Thank you so much for dive deep deep down on this issue.

I will push a commit that rollbacks the version of lock file to v1.
Is there anything else I should do for it?

Probably we should update the README as below to specify the versions? WTYT?
https://github.com/kubeflow/katib/tree/master/pkg/new-ui/v1beta1#requirements
image
=> node (v12.18.1) and npm (v6.13)

@andreyvelich
When is the due date to finish the PR to get it included in Kubeflow 1.5?

@kimwnasptd
Copy link
Member

Since this PR is entitled to bump Angular version not Node and npm, I think we better stick with node v12 and npm 6.
For Node and npm, we will need separated issue to work only if we need.

I completely agree!

I will push a commit that rollbacks the version of lock file to v1.
Probably we should update the README as below to specify the versions? WTYT?
https://github.com/kubeflow/katib/tree/master/pkg/new-ui/v1beta1#requirements

Perfect, these two would be awesome! We should be good to merge afterwards.

@andreyvelich
When is the due date to finish the PR to get it included in Kubeflow 1.5?

The currently proposed date is Jan 13 kubeflow/community#538 for the beginning of Feature Freeze. At that date the WGs should have a pass and pick which PRs, under review, should be merged for the next two weeks and also be very very conservative with adding new features.

But we are almost ready for merging this, so I don't think we'll have an issue with this PR

@andreyvelich
Copy link
Member

Thanks for this information!
if we want to include this change to KF 1.5, we should merge it before January 15th.

@google-oss-prow google-oss-prow bot removed the lgtm label Dec 14, 2021
@seong7
Copy link
Contributor Author

seong7 commented Dec 14, 2021

@kimwnasptd
Sorry for the delay.
I think this PR is ready to be merged now.
Please take a look into the 2 commits.

Thank you !

@kimwnasptd
Copy link
Member

All tests have passed, so we should be good to go. Thank you so much for your effort @seong7, this was great work!

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Dec 14, 2021
@kimwnasptd
Copy link
Member

@andreyvelich this will also need an /approve from you as well, since it touches the README of the app

@seong7
Copy link
Contributor Author

seong7 commented Jan 2, 2022

@andreyvelich Can we proceed this PR before January 15th?

@andreyvelich
Copy link
Member

@andreyvelich Can we proceed this PR before January 15th?

Sorry for the long reply @seong7.
I need some time to review the new changes.

I am on holidays until end of this week, and I will take a look at this PR on the next week.

@seong7
Copy link
Contributor Author

seong7 commented Jan 6, 2022

Thank you for replying @andreyvelich .
No problem.

@andreyvelich
Copy link
Member

Overall, this PR looks good.
@seong7 Do we need to keep all these fonts under /assets/fonts ?

@seong7
Copy link
Contributor Author

seong7 commented Jan 12, 2022

Overall, this PR looks good. @seong7 Do we need to keep all these fonts under /assets/fonts ?

I don't think we need those font files, but it keeps being generated automatically on npm i.
I've done some research, but I can't figure out what makes these font files being generated.

@andreyvelich
Copy link
Member

@seong7 As we discussed on the community meeting with @kimwnasptd we can add the font folder under .gitignone.
It is not necessary to have these files under Katib repository.

@google-oss-prow google-oss-prow bot removed the lgtm label Jan 13, 2022
@seong7
Copy link
Contributor Author

seong7 commented Jan 13, 2022

@seong7 As we discussed on the community meeting with @kimwnasptd we can add the font folder under .gitignone.
It is not necessary to have these files under Katib repository.

@andreyvelich Thank you for the quick decision making. Please have look into the new commit.

@andreyvelich
Copy link
Member

Thank you for the update @seong7!
Feel free to unhold this PR.
/lgtm
/approve
/cc @kimwnasptd

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, kimwnasptd, seong7

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:

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

@seong7
Copy link
Contributor Author

seong7 commented Jan 13, 2022

Thank you @andreyvelich and @kimwnasptd for guiding me through this long process.
Couldn't have made it without your help!

/unhold

@google-oss-prow google-oss-prow bot merged commit ce5421e into kubeflow:master Jan 13, 2022
@seong7 seong7 deleted the bump-angular-version-to12 branch January 13, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ui] Bump Angular version to 12
6 participants