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

[Build] Add OpenSearch Dashboards to Jenkinsfile #717

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Oct 8, 2021

Description

Creates OpenSearch Dashboards Jenkinsfile basically copying what was
in the existing OpenSearch Jenkinsfile. But skipping snapshots since
it was not doing anything with snapshots and publishing to S3
bucket paths of /builds/dashboards/ and /bundles/dashboards/ which
has not been verified yet. It was done this way, while keeping OpenSearch
to build to /builds and /bundles, to reduce the impact of adding
OpenSearch Dashboards.

For consistency, moved the root lvl Jenkinsfile which was only building
OpenSearch to jenkins/opensearch.

Signed-off-by: Kawika Avilla [email protected]

Issues Resolved

#620

Check List

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

@dblock
Copy link
Member

dblock commented Oct 8, 2021

My concerns are:

We are branching with if between OpenSearch and OpenSearch Dashboards. There's relatively little similar between the two other than the tooling. I don't think we have a snapshot build for OpenSearch Dashboards either. This tells me that we want separate Jenkins pipelines for different products - let's break this up?

@peternied @peterzhuamazon What do you think this should look like in terms of files/includes/libraries?

@kavilla
Copy link
Member Author

kavilla commented Oct 8, 2021

let's break this up?

Sure, I originally had it split.

I can dump it into jenkins/opensearch-dashboards and will leave the root Jenkinsfile untouched.

@dblock
Copy link
Member

dblock commented Oct 8, 2021

We should have mirroring pipelines for OpenSearch vs. OpenSearch Dashboards, so if you make this jenkins/OpenSearch-Dashboards/Jenkinsfile, move the OpenSearch one as well.

Creates OpenSearch Dashboards Jenkinsfile basically copying what was
in the existing OpenSearch Jenkinsfile. But skipping snapshots since
it was not doing anything with snapshots and publishing to S3
bucket paths of `/builds/dashboards/` and `/bundles/dashboards/` which
has not been verified yet. It was done this way, while keeping OpenSearch
to build to `/builds` and `/bundles`, to reduce the impact of adding
OpenSearch Dashboards.

For consistency, moved the root lvl Jenkinsfile which was only  building
OpenSearch to `jenkins/opensearch`.

Issue resolved:
opensearch-project#620

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla force-pushed the avillk/jenkins_osd branch from d041e18 to 879de42 Compare October 11, 2021 06:59
@kavilla kavilla marked this pull request as ready for review October 11, 2021 07:01
@kavilla kavilla linked an issue Oct 11, 2021 that may be closed by this pull request
@kavilla
Copy link
Member Author

kavilla commented Oct 11, 2021

If this goes in, existing Jenkins pipelines will need to be updated.

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.

rename underscore opensearch_dashboards to opensearch-dashboards

jenkins/opensearch_dashboards/Jenkinsfile Outdated Show resolved Hide resolved
jenkins/opensearch_dashboards/Jenkinsfile Outdated Show resolved Hide resolved
def artifactPath = "${manifest.build.version}/${OPENSEARCH_BUILD_ID}/${manifest.build.architecture}";

withAWS(role: 'opensearch-bundle', roleAccount: "${AWS_ACCOUNT_PUBLIC}", duration: 900, roleSessionName: 'jenkins-session') {
s3Upload(file: 'artifacts', bucket: "${ARTIFACT_BUCKET_NAME}", path: "builds/dashboards/${artifactPath}")
Copy link
Member

Choose a reason for hiding this comment

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

is there any intermediate output for dashboards in builds that's useful? In opensearch this is artifacts being published to maven for example; so do we need to upload it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can build the "snapshots" which would does not include the --release flag to the build here. Should publish that as well to the S3 bucket?

@dblock
Copy link
Member

dblock commented Oct 11, 2021

cc: @zelinh, in this PR there's some convention introduced for separating OpenSearch vs. OpenSearch Dashboards build, I don't want to end up with 3 locations, where are we with #669?

script {
// Stages must be explicitly added to prevent overwriting
// See https://ryan.himmelwright.net/post/jenkins-parallel-stashing/
def stages = ['build-x86', 'build-arm64']
Copy link

Choose a reason for hiding this comment

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

there is no x64?

Copy link
Member

Choose a reason for hiding this comment

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

x86 is x64 in this context because @mch2 did not change the name on the 1st place.

Copy link
Member

Choose a reason for hiding this comment

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

Let's fix it in both please.

@zelinh
Copy link
Member

zelinh commented Oct 11, 2021

I just started with #669 but haven't implemented. I agree we should have consistency for naming to separate OpenSearch and OpenSearch Dashboard.

@kavilla
Copy link
Member Author

kavilla commented Oct 12, 2021

@dblock:

rename underscore opensearch_dashboards to opensearch-dashboards

I copied the format of src/build_workflow/opensearch_dashboards should we modify that as well?

Handling the feedback without any question:

* Remove JAVA arg since it is unneeded for OpenSearch Dashboards.
* Change from x86 to x64
* Change S3 bucket from dashboards to opensearch-dashboards

Signed-off-by: Kawika Avilla <[email protected]>
@dblock
Copy link
Member

dblock commented Oct 12, 2021

rename underscore opensearch_dashboards to opensearch-dashboards

I copied the format of src/build_workflow/opensearch_dashboards should we modify that as well?

That one is a source code namespace, while this is a name of a product, albeit lowercase.

Update from feedback with potential to require more discussion.
* Rename folder from jenkins/opensearch_dashboards to jenkins/opensearch-dashboards
* Build ID from OPENSEARCH_BUILD_ID to OPENSEARCH_DASHBOARDS_BUILD_ID

Signed-off-by: Kawika Avilla <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2021

Codecov Report

Merging #717 (27ab227) into main (3b57cab) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #717   +/-   ##
=======================================
  Coverage   87.59%   87.59%           
=======================================
  Files          72       72           
  Lines        1919     1919           
=======================================
  Hits         1681     1681           
  Misses        238      238           
Impacted Files Coverage Δ
src/build_workflow/build_target.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2a65f0...27ab227. Read the comment docs.

@kavilla
Copy link
Member Author

kavilla commented Oct 12, 2021

cc: @zelinh, in this PR there's some convention introduced for separating OpenSearch vs. OpenSearch Dashboards build, I don't want to end up with 3 locations, where are we with #669?

so this issue blocked by #669, 669 implies the build path as well.

@@ -27,7 +27,7 @@ def __init__(
build_id=None,
output_dir="artifacts",
):
self.build_id = os.getenv("OPENSEARCH_BUILD_ID") or build_id or uuid.uuid4().hex
self.build_id = os.getenv("OPENSEARCH_BUILD_ID") or os.getenv("OPENSEARCH_DASHBOARDS_BUILD_ID") or build_id or uuid.uuid4().hex
Copy link
Member

Choose a reason for hiding this comment

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

This definitely tells me that we should merge these into a single variable. Then I think there was a comment on why we rename BUILD_NUMBER to OPENSEARCH_DASHBOARDS_BUILD_ID. Feels like all these things could be collapsed into a single BUILD_NUMBER. Wouldn't it make things a lot simpler?

Plus we'll also refactor Jenkinsfile into a shared library at some point I'm sure to avoid all this code duplication.

@dblock
Copy link
Member

dblock commented Oct 12, 2021

so this issue blocked by #669, 669 implies the build path as well.

I am ok with merging this and doing #699 on top.

@dblock dblock requested a review from peternied October 12, 2021 11:38
@dblock
Copy link
Member

dblock commented Oct 12, 2021

When I merge this, is everything going to break given that the OpenSearch Jenklinsfile moved? How do we get this pipeline into Jenkins?

@peternied
Copy link
Member

When I merge this, is everything going to break given that the OpenSearch Jenklinsfile moved? How do we get this pipeline into Jenkins?

When the job configuration is managed in source code this will be more possible, but that code will live in the CI repo and require its own deployment which will still result in some down time. We are effectively breaking the public interface of this system every time we rename/move these files, which I assume at some point we will do that less

@dblock
Copy link
Member

dblock commented Oct 12, 2021

When I merge this, is everything going to break given that the OpenSearch Jenklinsfile moved? How do we get this pipeline into Jenkins?

When the job configuration is managed in source code this will be more possible, but that code will live in the CI repo and require its own deployment which will still result in some down time. We are effectively breaking the public interface of this system every time we rename/move these files, which I assume at some point we will do that less

So YOLO.

@dblock dblock merged commit bca209e into opensearch-project:main Oct 12, 2021
@peternied
Copy link
Member

When I merge this, is everything going to break given that the OpenSearch Jenklinsfile moved? How do we get this pipeline into Jenkins?

I've updated the jenkins path for opensearch build project on our jenkins instance.

@kavilla kavilla deleted the avillk/jenkins_osd branch October 22, 2021 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run OpenSearch Dashboards build in Jenkins
7 participants