Copy manifest and bundle from S3 to jenkins #316

meghasaik · 2021-08-25T17:29:24Z

Description

[Describe what this change achieves]
This change basically achieves two things:

Copying the files from S3
Putting the files into S3

Issues Resolved

[List any issues this PR will resolve]
Issue No. 260
#260

Check List

Signed-off-by: Megha Sai Kavikondala [email protected]
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.

…ucket.

dblock

This needs to be called by something before we merge.
Dependencies need to be added into Pipfile.
It needs some OO, create a class that represents a bucket/destination and implement put/get methods on that. Maybe a Builder-like pattern, maybe Publisher that we use to collect results?
The code swallows exceptions and fails silently, that can't be good.
Add tests and documentation of where/how this is done in README's.

peternied · 2021-08-26T19:54:43Z

Could the use cases for this code described in Jenkins pipeline DSL? There is a built-in function that is being used by the build system for many common tasks so they don't need to be reimplemented https://www.jenkins.io/doc/pipeline/steps/pipeline-aws/

E.g. https://github.com/mch2/opensearch-build/blob/609d7163052d877f843f76582416f72229f1da71/bundle-workflow/Jenkinsfile#L64

…verting an op file to HTML file and then calling another function to put it inside an S3 bucket.

Change made for pulling and adding few changes

owaiskazi19 · 2021-08-27T23:47:37Z

bundle-workflow/src/getfiles_S3bkt.py

+
+        return args
+
+    def get_S3files(self, args):


Let's name it as download_files_from_S3

owaiskazi19 · 2021-08-27T23:48:54Z

bundle-workflow/src/read_write_s3.py

+import boto3
+
+
+class Read_Write_Files:


class name should be like ReadWriteFiles.
Ref: https://www.python.org/dev/peps/pep-0008/#class-names

owaiskazi19 · 2021-08-27T23:54:19Z

bundle-workflow/Pipfile

@@ -9,6 +9,7 @@ requests = "~=2.26"
 isort = "~=5.9"
 flake8 = "~=3.9"
 pytest = "*"
+boto3 = "*"


Can we assign it with the latest stable version and not *?

owaiskazi19 · 2021-08-27T23:56:42Z

bundle-workflow/src/getfiles_S3bkt.py

@@ -0,0 +1,36 @@
+# !/usr/bin/env python3


Name this file as download_files_from_S3 or something similar following the same naming convention

owaiskazi19

Follow naming convention as mentioned here: https://www.python.org/dev/peps/pep-0008/#prescriptive-naming-conventions

peternied

The surface area of this pull request has moved considerable from its initial revision. By separating many changes into smaller pull requests this will allow for faster and better feedback to get approval on the pull requests.

I would recommend the follow kinds of pull requests for the work I am seeing done here:

Overall workflow updates - when where is this code called and how does it tie back to the test systems, codifies the high level requirements, e.g. aws permissions, data in/out contracts
Remote File interactions
File parsing mechanics
Reporting UX
Transformation of local data into reporting format

setiah · 2021-08-30T17:03:56Z

bundle-workflow/src/read_write_s3.py

+    # get_S3Objects:Used for getting the files from S3
+    # bucket_name: Name of the bucket you want to get the file from
+    # role_arn: Amazon Resource Name (ARN) of the role that you want to assume
+    # role_session_name: Name for the session
+    # bucket_bundle: Object path from where you want to get the bundle
+    # bucket_manifest: Object path from where you want to get the manifest
+    # path_to_save_bundle: Path where you want to save the bundle at
+    # path_to_save_manifest: Path where you want to save the manifest at


Could you add these comments as pydoc

"""
"""

I have added them in the
"""
"""
pydoc block.

setiah · 2021-08-30T17:04:25Z

bundle-workflow/src/read_write_s3.py

+    # put_S3Objects:Used for putting the files into S3
+    # file_name: The file that you want to put in the S3 bucket
+    # file_path: The Object path where you want to save the file


add these comments as pydocs

setiah · 2021-08-30T17:08:47Z

bundle-workflow/src/get_files_from_S3.py

+
+
+import argparse
+


remove extra space

VachaShah

This needs tests and documentation to show how the functionality can be used.

VachaShah · 2021-08-30T17:27:09Z

bundle-workflow/src/read_write_s3.py

+        self.role_arn = role_arn
+        self.role_session_name = role_session_name
+
+    def get_S3_objects(self, bucket_name, bucket_bundle, bucket_manifest, path_to_save_bundle, path_to_save_manifest):


Is getting the manifest mandatory for this method? What if I just have an S3 path and want to get the files that are in that bucket, for example for DependencyInstaller. Currently the code here is assuming that all the user passes in all the parameters.

I have modified the method to take only the bundle parameter where through the S3 path we can get the files that are in that bucket

VachaShah · 2021-08-30T17:35:32Z

bundle-workflow/src/read_write_s3.py

+            client.download_file(bucket_name, bucket_bundle, path_to_save_bundle)
+            client.download_file(bucket_name, bucket_manifest, path_to_save_manifest)
+        except:
+            print("An exception occurred. No file is present in the bucket.")


There might be other valid exceptions which need to be raised, currently for all the exceptions, this is printing a "no file present" error message.

I have modified it by printing out the exception that has occurred as either of the three exceptions might occur which are 1.bucket name does not exist 2. Not a directory for the object path 3. local file path when not given properly

VachaShah · 2021-08-30T17:36:09Z

bundle-workflow/src/read_write_s3.py

+        try:
+            client.upload_file(file_name, bucket_name, file_path)
+        except:
+            print("An exception occurred. No file found for uploading.")


Please see my comment for exceptions above.

setiah · 2021-08-30T17:46:41Z

bundle-workflow/src/read_write_s3.py

+        self.role_arn = role_arn
+        self.role_session_name = role_session_name
+
+    def get_S3_objects(self, bucket_name, bucket_bundle, bucket_manifest, path_to_save_bundle, path_to_save_manifest):


We need to make this more generic to achieve two use cases -

Download a particular S3 object

Download all maven dependencies from a given s3 location

I have modified it now, as the bundle_path arg will be used for downloading the entire file/folder.

setiah · 2021-08-30T17:49:30Z

bundle-workflow/src/read_write_s3.py

+    def __init__(self, role_arn, role_session_name):
+        self.role_arn = role_arn
+        self.role_session_name = role_session_name


Make the role parameters optional. Abstract the role passing part from caller, by using defaults from environment vars (for now) to read/write from s3. By keeping these optional, the caller can choose to pass these for specific use cases, while the defaults work for most

The role parameters will be optional and the default values will there for these parameters to take their values from.

… name, added comments and also made changes to download the folder

dblock

needs tests

dblock · 2021-08-31T02:39:22Z

bundle-workflow/src/read_write_s3.py

+import boto3
+
+
+class ReadWriteFiles:


This should represent a thing, e.g. S3 or S3Bucket and take bucket as a parameter since it's required in both put/get.

dblock · 2021-08-31T02:40:09Z

bundle-workflow/src/read_write_s3.py

+        self.role_arn = role_arn
+        self.role_session_name = role_session_name
+
+    def get_S3_objects(self, bucket_name, bucket_bundle, path_to_save_bundle=None):


Rename to get or download.

The parameters bucket_bundle and path_to_save_bundle don't make sense in S3 context. Should be prefix and dest.

What's a None dest? I would just require it.

dblock · 2021-08-31T02:40:21Z

bundle-workflow/src/read_write_s3.py

+                    continue
+                my_bucket.download_file(obj.key, target)
+        except Exception as e:
+            print(e)


Don't swallow exception, let it get raised.

dblock · 2021-08-31T02:40:47Z

bundle-workflow/src/read_write_s3.py

+    file_name: The file that you want to put in the S3 bucket
+    file_path: The Object path where you want to save the file
+    """
+    def put_S3_objects(self, bucket_name, file_name, file_path):


Rename to put or upload.

dblock · 2021-08-31T02:44:06Z

bundle-workflow/src/read_write_s3.py

+
+        try:
+            client.upload_file(file_name, bucket_name, file_path)
+        except Exception as e:


Don't swallow exception.

dblock · 2021-08-31T02:45:06Z

bundle-workflow/src/read_write_s3.py

+            RoleSessionName=self.role_session_name,
+            DurationSeconds=3600)['Credentials']
+
+        boto3.client('s3',


This code is the same between get and put. Extract into the constructor that creates a new client for a bucket.

setiah · 2021-09-03T18:37:24Z

This is replaced with #367

meghasaik added 11 commits August 25, 2021 08:57

Added the methods for getting as well as putting files from/into s3 b…

6a53d10

…ucket.

Removed lines which must not be present

0816534

Import changes made

586b5ae

Changes

4546d81

Changes made by adding licenses.

44e346a

Style changes and comments made

60af2d2

Style changes made

bff79a1

Style changes made.

b703da6

Style changes-3

edb7b82

Style changes-4

264ff49

Style changes-5

c641d8c

meghasaik changed the title ~~Created methods for 1. copying the files from S3 to jenkins and 2. putting the files to S3~~ Copy manifest and bundle from S3 to jenkins Aug 25, 2021

meghasaik requested review from setiah, VachaShah and saratvemulapalli August 25, 2021 17:32

dblock requested changes Aug 25, 2021

View reviewed changes

Merge branch 'opensearch-project:main' into main

6d03c20

meghasaik and others added 2 commits August 27, 2021 09:31

Merge branch 'opensearch-project:main' into main

488d5c0

Made changes by adding a file that takes 6 cmdline arguements for con…

a8ec4f8

…verting an op file to HTML file and then calling another function to put it inside an S3 bucket.

meghasaik linked an issue Aug 27, 2021 that may be closed by this pull request

Publish human-readable test run reports as a simple HTML document #130

Closed

meghasaik removed a link to an issue Aug 27, 2021

Publish human-readable test run reports as a simple HTML document #130

Closed

meghasaik and others added 8 commits August 27, 2021 11:25

Made changes reagrding to code simplification.

3515032

Merge branch 'opensearch-project:main' into main

f17df2d

Merge branch 'main' of https://github.com/meghasaik/opensearch-build

94282c7

Change made for pulling and adding few changes

Changes made for imports

6d03940

Sorted imports

5ba5195

sorted import 1

06a4cec

sorted import 1

115e553

sorted import 1

fac473d

owaiskazi19 reviewed Aug 27, 2021

View reviewed changes

owaiskazi19 suggested changes Aug 27, 2021

View reviewed changes

meghasaik added 2 commits August 27, 2021 17:17

Made changes related to naming conventions

5ff63d0

Changes made related to pipfile.lock

1a81a7e

meghasaik requested review from owaiskazi19 and setiah August 28, 2021 00:23

peternied suggested changes Aug 28, 2021

View reviewed changes

meghasaik force-pushed the main branch from f912724 to 1a81a7e Compare August 30, 2021 16:36

setiah reviewed Aug 30, 2021

View reviewed changes

bundle-workflow/src/get_files_from_S3.py

import argparse

Copy link

Contributor

setiah Aug 30, 2021

Choose a reason for hiding this comment

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

remove extra space

VachaShah reviewed Aug 30, 2021

View reviewed changes

VachaShah mentioned this pull request Aug 30, 2021

Added DependencyInstaller to copy dependencies from s3 to maven local #318

Merged

1 task

setiah reviewed Aug 30, 2021

View reviewed changes

meghasaik added 2 commits August 30, 2021 17:07

Changes made by adding default values for the assume role and session…

e1b2f7e

… name, added comments and also made changes to download the folder

Made style changes

57bf946

meghasaik requested review from VachaShah and setiah August 31, 2021 00:14

Made changes related to taking the env variable for the role_assume

324a774

dblock requested changes Aug 31, 2021

View reviewed changes

meghasaik added 2 commits August 31, 2021 13:06

Made changes to the both the files given.

7637e66

Changes made in accordance to exceptions to be raised

7c4bb32

This was referenced Sep 1, 2021

S3 utility methods for reading and writing manifests, bundles, and logs in Jenkins #367

Merged

[META] Test orchestration framework #122

Closed

setiah closed this Sep 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy manifest and bundle from S3 to jenkins #316

Copy manifest and bundle from S3 to jenkins #316

meghasaik commented Aug 25, 2021

dblock left a comment •

edited

Loading

peternied commented Aug 26, 2021

owaiskazi19 Aug 27, 2021 •

edited

Loading

owaiskazi19 Aug 27, 2021

owaiskazi19 Aug 27, 2021

owaiskazi19 Aug 27, 2021 •

edited

Loading

owaiskazi19 left a comment •

edited

Loading

peternied left a comment

setiah Aug 30, 2021

meghasaik Aug 31, 2021

setiah Aug 30, 2021

setiah Aug 30, 2021

VachaShah left a comment

VachaShah Aug 30, 2021

meghasaik Aug 31, 2021

VachaShah Aug 30, 2021

meghasaik Aug 31, 2021

VachaShah Aug 30, 2021

setiah Aug 30, 2021

meghasaik Aug 31, 2021

setiah Aug 30, 2021

meghasaik Aug 31, 2021

dblock left a comment

dblock Aug 31, 2021

dblock Aug 31, 2021

dblock Aug 31, 2021

dblock Aug 31, 2021

dblock Aug 31, 2021

dblock Aug 31, 2021

dblock Aug 31, 2021

setiah commented Sep 3, 2021

Copy manifest and bundle from S3 to jenkins #316

Copy manifest and bundle from S3 to jenkins #316

Conversation

meghasaik commented Aug 25, 2021

Description

Issues Resolved

Check List

dblock left a comment • edited Loading

Choose a reason for hiding this comment

peternied commented Aug 26, 2021

owaiskazi19 Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

owaiskazi19 left a comment • edited Loading

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VachaShah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setiah commented Sep 3, 2021

dblock left a comment •

edited

Loading

owaiskazi19 Aug 27, 2021 •

edited

Loading

owaiskazi19 Aug 27, 2021 •

edited

Loading

owaiskazi19 left a comment •

edited

Loading