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

Updates demo script to execute with bundled jdk #3777

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Nov 30, 2023

Description

This PR adds changes to the tool to allow execution with bundled jdk. It also refactors usage of static variables.
It also addresses a small bug in isStringAlreadyPresentFile to modify from regex match to "startsWith".

Tests are added as fast-follow in #3807

Check List

- [ ] New functionality includes testing <- fast-follow via #3807
- [ ] New functionality has been documented

  • 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.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
… classes and configuration

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura DarshitChanpura force-pushed the demo-config-java-tool-tests branch from e6e3b89 to 2cc8ba4 Compare November 30, 2023 17:12
…luster

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

These tests look pretty brittle since they are handling output written to the console and files, I'd recommend breaking up how the parts of the tool work so you can have easier to maintain test cases.

Something to consider if the tool gets a new option, say <10 lines of code. In the tests you would only really need two new test cases (pass/fail) and no unrelated tests would break.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura DarshitChanpura force-pushed the demo-config-java-tool-tests branch from fb9768d to 50c67bb Compare November 30, 2023 20:35
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Copy link
Member Author

Looking into windows failures now.

Verified

This commit was signed with the committer’s verified signature.
bartekpacia Bartek Pacia
…s not set

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…demo install script

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…demo install script

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura DarshitChanpura changed the title Adds tests for demo configuration java tool Updates demo script to execute with bundled jdk and adds tests for demo configuration java tool Dec 1, 2023
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Took another pass. Nice work getting SnakeYaml to write to opensearch.yml!

Verified

This commit was signed with the committer’s verified signature.
bartekpacia Bartek Pacia
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Copy link
Member Author

Code Coverage will be enhanced in #3807

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura DarshitChanpura force-pushed the demo-config-java-tool-tests branch from 36c5d8a to 5fbfdad Compare December 12, 2023 05:16
@derek-ho
Copy link
Collaborator

Ship it 🚢

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
cwperks
cwperks previously approved these changes Dec 12, 2023
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

This PR LGTM.

One additional comment: Should the SecuritySettingsConfigurer get the values it needs in its constructor instead of the entire installer object?

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Copy link
Member Author

One additional comment: Should the SecuritySettingsConfigurer get the values it needs in its constructor instead of the entire installer object?

I started out with that but thought it might be better to pass just one reference installer rather than multiple local copies of variables inside Installer class. IMO, I'd prefer to keep it as is.

cwperks
cwperks previously approved these changes Dec 12, 2023
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks DC

@DarshitChanpura DarshitChanpura merged commit 06d8c29 into opensearch-project:main Dec 12, 2023
80 of 82 checks passed
DarshitChanpura added a commit to DarshitChanpura/security that referenced this pull request Dec 13, 2023
)



Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
cwperks pushed a commit that referenced this pull request Dec 18, 2023
…t for Bundled JDK for this tool and updates DEVELOPER_GUIDE.md (#3845)

### Description
Backports following commits related to demo configuration tool from main
to 2.x:

- [x]
[17748b9](17748b9)
from #3669
- [x]
[4496440](4496440)
from #3734
- [x]
[06d8c29](06d8c29)
from #3777
- [x]
[e698315](e698315)
from #3807
- [x]
[9d11524](9d11524)
from #3843
- [x]
[62aed21](62aed21)
from #3850
- [x]
[ceabe13](ceabe13)
from #3844

### Issues Resolved
- Related to #3827


### Testing
- automated tests

### Check List
- [x] New functionality includes testing
- [x] New functionality has been documented
- [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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
prabhask5 pushed a commit to prabhask5/opensearch-security that referenced this pull request Jan 11, 2024
)

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
dlin2028 pushed a commit to dlin2028/security that referenced this pull request May 1, 2024
)



Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants