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

Add JAVA_HOME arguments in manifest workflow from template manifests. #2358

Merged
merged 6 commits into from
Jul 27, 2022

Conversation

zelinh
Copy link
Member

@zelinh zelinh commented Jul 21, 2022

Description

Add Java home argument to the manifest workflow based on the manifest version based on its major version templates stored in the manifests folder. If it's 1.x (in our case 1.3.x primarily), it will add jdk 11 argument. If it's 2.x or 3.x, it will add jdk 17 for now. Otherwise, it will set to be corresponded with 2.x templates which has jdk17 by default.

Followup to the previous closed PR #2354

Issues Resolved

#2044

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.

zelinh added 3 commits July 19, 2022 13:28
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
@zelinh zelinh requested a review from dblock July 21, 2022 23:17
@zelinh zelinh requested a review from a team as a code owner July 21, 2022 23:17
@zelinh zelinh self-assigned this Jul 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2022

Codecov Report

Merging #2358 (040ca0f) into main (858dc0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2358   +/-   ##
=========================================
  Coverage     94.61%   94.62%           
  Complexity       27       27           
=========================================
  Files           209      209           
  Lines          4309     4316    +7     
  Branches         29       29           
=========================================
+ Hits           4077     4084    +7     
  Misses          226      226           
  Partials          6        6           
Impacted Files Coverage Δ
src/manifests_workflow/input_manifests.py 99.04% <100.00%> (+0.06%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

Signed-off-by: Zelin Hao <zelinhao@amazon.com>
@zelinh zelinh changed the title Manifest java home Add JAVA_HOME arguments in manifest workflow from template manifests. Jul 22, 2022
@zelinh
Copy link
Member Author

zelinh commented Jul 25, 2022

This PR is intended to refactor some of our hardcoding in our manifest_workflow and resolved the issue to add JAVA_HOME argument manually as what we did in this PR.

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This is good!

See below to make it a bit simpler.

Write tests for using the default manifest vs. using a manifest file that exists.

@@ -136,6 +136,8 @@ def __init__(self, data: dict) -> None:

def __to_dict__(self) -> dict:
return {
"name": self.name
} if self.args is None else {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to do this because to_dict should compact the outcome. Just return what you have in else.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dblock Actually I think we might need this. Otherwise it will output args: None in dashboard manifest it generated.

Copy link
Member

@dblock dblock Jul 26, 2022

Choose a reason for hiding this comment

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

Make sure we call to_dict, which removes these None values.

input_manifest_templates = InputManifest.from_file(open(template_full_path))
else:
input_manifest_templates = InputManifest.from_file(open(os.path.join(templates_base_path, self.prefix,
"2.x", "manifest.yml")))
Copy link
Member

Choose a reason for hiding this comment

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

This hard-coding is bound to cause problems.

  1. Use the entire manifest, not just the templates.ci part, so remove the default data.
  2. Add a default folder for the default manifest and load that instead of 2.x/manifest.yml.

Signed-off-by: Zelin Hao <zelinhao@amazon.com>
@zelinh zelinh force-pushed the manifest-java-home branch from 9efd62a to 26b57fe Compare July 27, 2022 01:15
@zelinh zelinh requested a review from dblock July 27, 2022 01:34
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I think there's one more problem, the manifests CI workflow is going to pickup these templates. They probably need to move or the folder needs to be excluded somehow.

https://github.com/opensearch-project/opensearch-build/blob/main/.github/workflows/manifests.yml#L20

src/manifests_workflow/component.py Outdated Show resolved Hide resolved
src/manifests_workflow/input_manifests.py Outdated Show resolved Hide resolved
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
@zelinh zelinh requested a review from dblock July 27, 2022 19:14
@dblock dblock merged commit cbf57ab into opensearch-project:main Jul 27, 2022
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.

4 participants