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.

Sign up for GitHub

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

Jump to bottom

Migrate angular to v9 #4930

Merged
merged 8 commits into from
Feb 28, 2020
Merged

Migrate angular to v9 #4930

merged 8 commits into from
Feb 28, 2020

Conversation

floreks
Copy link
Member

@floreks floreks commented Feb 20, 2020 •
edited
Loading

Fixes #4893

  • Updated CRD data to also use ace-editor component instead of custom implementation.
  • Removed ng2-ace-editor dependency and reused vanilla implementation.
  • Tested Firefox 73.0 on prod and I was able to log in correctly (related to Firefox can not login #4922). Will test once again after release and close if resolved.

I18n pipeline update

Angular 9 introduced a couple of changes to the localization framework.

Locale codes

New locale codes have to be used as described in: https://angular.io/guide/i18n#setting-up-the-locale-of-your-app

Example changes:
zh-cn -> zh-Hans
zh-tw -> zh-Hant

To be able to properly use zh-Hant-HK (codes with 3 parts) we have to wait for the issue angular/angular-cli#17032 to be fixed. I have already renamed directories, removed symlinks as new i18n pipeline do not seem to like them. I have had to also slightly modify the locale handler in the backend.

Frontend build

We do not need to build the frontend for every locale separately now. Localization configuration is defined in angular.json file and when --localize flag is passed to ng, localized frontend will be built.

@shu-mutou @zehuaiWANG please check if I have properly assigned new locale codes to languages.

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 20, 2020
@k8s-ci-robot k8s-ci-robot requested review from cheld and konryd February 20, 2020 13:06
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. language/de Updates or issues for German translations. labels Feb 20, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 20, 2020
@maciaszczykm
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2020
@floreks
Copy link
Member Author

floreks commented Feb 20, 2020

There seems to be an issue with prod build.

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/zh Updates or issues for Chinese translations. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 20, 2020
@floreks floreks force-pushed the bump/angular branch 2 times, most recently from 659032b to 6397825 Compare February 20, 2020 16:33
@maciaszczykm
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2020
@floreks floreks mentioned this pull request Feb 20, 2020
Closed
@shu-mutou
Copy link
Contributor

Locales except Chinese seem good. Since zh-Hans and zh-Hant aren't implemented in firefox and chrome, so only zh is actually available. I'm investigating the available way to use actual locale IDs.

@shu-mutou
Copy link
Contributor

I could not find the way to use zh-* locales that are sent via HTTP Accept-Language header today.... 😞

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2020
@floreks
Copy link
Member Author

floreks commented Feb 21, 2020 •
edited
Loading

@shu-mutou The new angular only supports new locale codes that are based on BCP47 regulations. Browsers should adapt them eventually. I was actually using language switcher plugin to test if they work and it is also using new locale codes. For backward compatibility, we could add a map to our backend that will try to resolve i.e. zh-cn as zh-Hans if an exact translation is not found. It seems like an easy solution and should work for the time being. In the frontend we can use new language codes.

@floreks
Copy link
Member Author

floreks commented Feb 27, 2020

I have fixed locale code mapping to also support old locales for now. I have also found a bug with default language package in Go and fixed locale matching logic.

It is now ready for tests. Once angular releases patch version for the mentioned issue, we can bump it and restore support for missing languages.

@floreks
Copy link
Member Author

floreks commented Feb 27, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2020
@maciaszczykm
Copy link
Member

I will test it later.

@maciaszczykm
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 28, 2020
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 28, 2020
@maciaszczykm
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks, maciaszczykm

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 [floreks,maciaszczykm]

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

@k8s-ci-robot k8s-ci-robot merged commit 4cab78c into kubernetes:master Feb 28, 2020
@floreks floreks deleted the bump/angular branch February 28, 2020 09:56
@floreks floreks mentioned this pull request Mar 3, 2020
Merged
@hwdef
Copy link
Member

hwdef commented Mar 23, 2020 •
edited
Loading

I just noticed this PR, I think it's confusing to change the file directory like this. Can we simplify zh-Hans-SG zh-Hans zh-Hant-HK zh-Hant and zh?

@floreks @shu-mutou @maciaszczykm

@shu-mutou
Copy link
Contributor

I'm tackling it at #4969. Please check it.

@hwdef
Copy link
Member

hwdef commented Mar 23, 2020

image

This is their structure. The ones in parentheses are Accept-Language.
Now we are missing tw and cn.
And many folders are duplicated, such as zh-hans-sg and zh-hans and zh.

If we want to distinguish by the smallest unit, we can keep the lowest directory. Which is reserved zh-hans-CN,zh-hans-SG,zh-hant-HK,zh-hant-TW. or zh-CN,zh-SG,zh-HK,zh-TW.

Or we can keep the middle level. zh-hans and zh-hant.Then we link CN,SG,HK,TW to them.This way we only need to translate two files. But I don't recommend this way. Because although HK and TW belong to zh-hant, they also have some differences.

@shu-mutou
Copy link
Contributor

I'm considering to keep 'zh-Hans', 'zh-Hant' and 'zh-Hant-HK' in #4969, not to duplicate large translation files. Could you review it?

@hwdef
Copy link
Member

hwdef commented Mar 23, 2020

@shu-mutou
I am sorry , I just noticed it. You discussed a lot and I need a little time to read.
Thanks for your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheld cheld Awaiting requested review from cheld

@konryd konryd Awaiting requested review from konryd

Assignees

@maciaszczykm maciaszczykm

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/de Updates or issues for German translations. language/zh Updates or issues for Chinese translations. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ConfigMap view breaks after changing
5 participants
@floreks @maciaszczykm @shu-mutou @k8s-ci-robot @hwdef