-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Deal with crosssite api call after login. #10533
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
Deal with crosssite api call after login. #10533
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.20 #10533 +/- ##
=========================================
Coverage 16.00% 16.00%
+ Complexity 13104 13103 -1
=========================================
Files 5651 5651
Lines 495841 495842 +1
Branches 60044 60044
=========================================
+ Hits 79365 79368 +3
Misses 407613 407613
+ Partials 8863 8861 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Vishesh <[email protected]>
|
@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 12718 |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12719 |
| SetCsLatestVersion ({ commit }, rolename) { | ||
| if (!vueProps.$config.notifyLatestCSVersion) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of Github, we should query one of the ASF or repo links to find latest release? For example - https://dlcdn.apache.org/cloudstack/releases/ - we may even introduce to put like a file that gives latest version info say from https://download.cloudstack.org/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohityadavcloud , I am not sure this would be easily cleanly implemented.
https://api.github.com/repos/apache/cloudstack/releases give a cleanish yaml with results.
In contrast https://dlcdn.apache.org/cloudstack/releases/ gives a directory listing.
https://download.cloudstack.org/ also gives a directory listing which is really messy.
I would prefer to continue as is but make the retrieval of new version optional, advising operators to set it on test environments and to disable it in production.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just put a file on cloudstack.apache.org, for example
https://cloudstack.apache.org/latest_cloudstack_version ?
We do not need to get the new version very soon after the release, 1-7 days delay is good I think.
shwstppr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the idea of making the version check configurable.
One issue I see with our current approach of storing fetched version in store is that if the admin logs in 50 times in an Incognito window it will query for version 50 times.
Maybe we can also improve the overall readability of SetCsLatestVersion
diff --git a/ui/src/store/modules/user.js b/ui/src/store/modules/user.js
index 6113f3ed94..5b724d1664 100644
--- a/ui/src/store/modules/user.js
+++ b/ui/src/store/modules/user.js
@@ -556,27 +556,32 @@ const user = {
commit('SET_DOMAIN_STORE', domainStore)
},
SetCsLatestVersion ({ commit }, rolename) {
- const lastFetchTs = store.getters.latestVersion?.fetchedTs ? store.getters.latestVersion.fetchedTs : 0
- if (rolename === 'Root Admin' && (+new Date() - lastFetchTs) > 24 * 60 * 60 * 1000) {
- axios.get(
- 'https://api.github.com/repos/apache/cloudstack/releases'
- ).then(response => {
- let latestReleaseVersion = getParsedVersion(response[0].tag_name)
- let latestTag = response[0].tag_name
-
- for (const release of response) {
- if (release.tag_name.toLowerCase().includes('rc')) {
- continue
- }
- const parsedVersion = getParsedVersion(release.tag_name)
- if (semver.gte(parsedVersion, latestReleaseVersion)) {
- latestReleaseVersion = parsedVersion
- latestTag = release.tag_name
- commit('SET_LATEST_VERSION', { version: latestTag, fetchedTs: (+new Date()) })
- }
- }
- }).catch(ignored => {})
+ if (!vueProps.$config.notifyLatestCSVersion) {
+ return
}
+ const lastFetchTs = store.getters.latestVersion?.fetchedTs || 0
+ if (rolename !== 'Root Admin' || (+new Date() - lastFetchTs) <= 24 * 60 * 60 * 1000) {
+ return
+ }
+ let lastFetchedVersion = store.getters.latestVersion?.version || '0.0.0'
+ const parsedLastFetchedVersion = getParsedVersion(lastFetchedVersion)
+ axios.get(
+ 'https://api.github.com/repos/apache/cloudstack/releases'
+ ).then(response => {
+ for (const release of response) {
+ if (release.tag_name.toLowerCase().includes('rc') || release.draft || release.prerelease) {
+ continue
+ }
+ const parsedVersion = getParsedVersion(release.tag_name)
+ if (semver.gte(getParsedVersion(release.tag_name), parsedLastFetchedVersion)) {
+ lastFetchedVersion = release.tag_name
+ break
+ }
+ }
+ }).catch(ignored => {
+ }).finally(() => {
+ commit('SET_LATEST_VERSION', { version: lastFetchedVersion, fetchedTs: (+new Date()) })
+ })
},
SetDarkMode ({ commit }, darkMode) {
commit('SET_DARK_MODE', darkMode)
| const result = response.listusersresponse.user[0] | ||
| commit('SET_INFO', result) | ||
| commit('SET_NAME', result.firstname + ' ' + result.lastname) | ||
| store.dispatch('SetCsLatestVersion', result.rolename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the issue where it is making 2 API calls.
|
Good feature, but anything that makes CloudStack rely on external resources should be disable-able. Some/Many corporate environments are run in closed down intranets where this is causing UI errors and generating reports from users. I know of at least 2 such situations. |
|
Tested this PR. This works as expected. When |
thanks @vishesh92 |
In case of failures, it was sent twice earlier. Now it's sent only once. |
weizhouapache
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
thanks @vishesh92
can you approve it ?
Description
This PR...
Fixes: #10522
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?