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

Remove user from the responses #201

Merged
merged 3 commits into from
Oct 15, 2021

Conversation

skkosuri-amzn
Copy link
Contributor

@skkosuri-amzn skkosuri-amzn commented Oct 11, 2021

Issue #, if available:
Remove user from the responses

Description of changes:
Remove user from the responses

All unit and integ passed. All security tests also passed. ./gradlew :alerting:integTest -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername=opensearch -Dhttps=true -Dsecurity=true -Duser=admin -Dpassword=admin

CheckList:
[x] Commits are signed per the DCO using --signoff

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.

Assert.assertTrue(alerts.single().errorMessage?.contains("The destination address is invalid") as Boolean)
Assert.assertNotNull(alerts.single().errorMessage)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check being modified? It doesn't seem like the error message should have changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the flaky test, not really part of user object changes. I have this test failing sometimes when security is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok that makes sense. Thanks.

Copy link
Contributor

@qreshi qreshi Oct 13, 2021

Choose a reason for hiding this comment

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

If this fails sometimes but your assertNotNull check doesn't, it means that there is an error message but not the one we expect. That would mean the Alert is failing for another reason. Did you get a chance to see what the error was in these scenarios?

Copy link
Contributor

Choose a reason for hiding this comment

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

@skkosuri-amzn By the way, I realized even as far back as our plugin version being 1.0.0, the Pull and Run Docker step in our Multi-node test workflow has been failing with Error response from daemon: manifest for opensearchstaging/opensearch:<version> not found: manifest unknown: manifest unknown. Is that a known issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skkosuri-amzn By the way, I realized even as far back as our plugin version being 1.0.0, the Pull and Run Docker step in our Multi-node test workflow has been failing with Error response from daemon: manifest for opensearchstaging/opensearch:<version> not found: manifest unknown: manifest unknown. Is that a known issue?

I am not aware of that. Looks like its a different one based on above error msg.

@skkosuri-amzn skkosuri-amzn merged commit e0e41df into opensearch-project:main Oct 15, 2021
@AWSHurneyt AWSHurneyt added the v1.2.0 Targeted for 1.2.0 release label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.2.0 Targeted for 1.2.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants