-
Notifications
You must be signed in to change notification settings - Fork 164
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
Bumps version of main to 2.0.0.0 #928
Bumps version of main to 2.0.0.0 #928
Conversation
main
to 2.0.0.0
main
to 2.0.0.0
This build requires |
The test-build seems to be failing because it needs node version upgrade to [email protected], and the PR: #924 is failing because it needs version to be upgraded to 2.0.0.0 which inherently creates a deadlock. How should we proceed @opensearch-project/security ? |
@DarshitChanpura Can we do both changes in the same PR? |
On top of node 14 upgrade, this still stands true. This build requires 2.0.0.0 artifact built for opensearch-security
Okay I will bring node14 upgrade changes in this PR. |
797ef16
to
4147816
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #928 +/- ##
==========================================
+ Coverage 71.98% 72.14% +0.15%
==========================================
Files 87 87
Lines 1906 1906
Branches 247 242 -5
==========================================
+ Hits 1372 1375 +3
+ Misses 480 477 -3
Partials 54 54 ☔ View full report in Codecov by Sentry. |
aad2111
to
f1a284e
Compare
f1a284e
to
8e6986d
Compare
I'm trying to debug Unit tests which fail due to mismatch in Jest snapshot matching: See here Meanwhile if you know anything about this error please let me know. |
I'm unable to reproduce it on local. All the unit tests pass when I run them in the same setup. Steps that I followed to run the unit tests:
|
1318b4e
to
c8efd79
Compare
@DarshitChanpura Looks like there are some unit tests failures, Seperately, I'm looking into the integration test failures, I know we will need a code change for at least one of them |
I have fixed unit tests and will push the change soon |
c8efd79
to
560069a
Compare
ca304c8
to
6a43aa6
Compare
Signed-off-by: Darshit Chanpura <[email protected]>
6a43aa6
to
80687fe
Compare
Signed-off-by: Darshit Chanpura <[email protected]>
80687fe
to
3da4006
Compare
We can re-run the integration tests job once security plugin is added to opensearch-build manifest for 2.0.0 |
@DarshitChanpura Note; this is still waiting on opensearch-project/opensearch-build#1924 to be merged and a new build has been produced |
5c39f76
to
715580c
Compare
common/index.ts
Outdated
@@ -44,5 +44,6 @@ export enum AuthType { | |||
*/ | |||
export function isValidResourceName(resourceName: string): boolean { | |||
// see: https://javascript.info/regexp-unicode | |||
return !/[\p{C}%]/u.test(resourceName) && resourceName.length > 0; | |||
const exp = new RegExp('[p{C}%]', 'u'); |
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.
I think this is going to match the p
character, I think you need \p{C}
if you want to make sure that we are looking for 'other' unicode characters
715580c
to
fc13901
Compare
…oves test workflow filter for branches Signed-off-by: Darshit Chanpura <[email protected]>
fc13901
to
01fc571
Compare
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.
Looks good!
@@ -16,7 +16,7 @@ exports[`Account navigation button renders 1`] = ` | |||
id="actionsMenu" | |||
isOpen={false} | |||
onClick={[Function]} | |||
ownFocus={false} | |||
ownFocus={true} |
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 this was asked before, but why this change?
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 is because the unit test were failing due to jest snapshot mismatch... this is something worth the discussion.
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.
Note, this control was likely modified by a change in the styles from OpenSearch-Dashboards. We should follow up and confirm that our UI looks clean and behaves as expected between 1.3.X and this 2.0.0 version.
I've created #938 for following up on this.
@@ -171,7 +171,7 @@ export class BasicAuthRoutes { | |||
? this.coreSetup.http.basePath.serverBasePath | |||
: '/'; | |||
const requestQuery = request.url.query as ParsedUrlQueryParams; | |||
if (requestQuery.nextUrl !== undefined) { | |||
if (requestQuery?.nextUrl !== undefined) { |
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.
I've created #936 to follow up, we should stop using the legacy url.query method
Description
Bumps version to 2.0.0.0
Issues resolved
Category
Maintenance
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.