-
Notifications
You must be signed in to change notification settings - Fork 133
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
Refactor cicd pipeline #764
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Matthew Fala <[email protected]>
9b66801
to
b687cc4
Compare
Signed-off-by: Matthew Fala <[email protected]>
b687cc4
to
2b89617
Compare
Signed-off-by: Matthew Fala <[email protected]>
# Clear up testing resources | ||
- python ./load_tests/load_test.py delete_testing_resources | ||
- | | ||
for i in {1..2}; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make this 2
as a configure retry parameter ?
for i in {1..2}; do | ||
python ./load_tests/load_test.py ${PLATFORM} && break \ | ||
|| ( | ||
if [ $i == 2 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same 2
here.
phases: | ||
install: | ||
runtime-versions: | ||
golang: 1.16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there 1.x ?? reduce future upgrade effort >
@@ -18,8 +18,8 @@ | |||
TESTING_RESOURCES_STACK_NAME = os.environ['TESTING_RESOURCES_STACK_NAME'] | |||
PREFIX = os.environ['PREFIX'] | |||
EKS_CLUSTER_NAME = os.environ['EKS_CLUSTER_NAME'] | |||
LOGGER_RUN_TIME_IN_SECOND = 600 | |||
BUFFER_TIME_IN_SECOND = 1000 | |||
LOGGER_RUN_TIME_IN_SECOND = int(os.getenv("LOAD_TEST_RUN_TIME_IN_SECONDS", "600")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we keep above ?? and do soemthing like
DEFAULT_LOGGER_RUN_TIME_IN_SECOND = 600
LOGGER_RUN_TIME_IN_SECOND = int(os.getenv("LOAD_TEST_RUN_TIME_IN_SECONDS", DEFAULT_LOGGER_RUN_TIME_IN_SECOND))
``
if (output == "cloudwatch" or output == "all"): | ||
# Delete associated cloudwatch log streams | ||
if os.getenv('CW_LOG_GROUP_NAME') != None: | ||
client = session.client('logs') | ||
response = client.describe_log_streams( | ||
logGroupName=os.getenv('CW_LOG_GROUP_NAME') | ||
) | ||
for stream in response["logStreams"]: | ||
client.delete_log_stream( | ||
logGroupName=os.environ['CW_LOG_GROUP_NAME'], | ||
logStreamName=stream["logStreamName"] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we pull this out into a separate method ?
s3 = session.resource('s3') | ||
bucket = s3.Bucket(os.environ['S3_BUCKET_NAME']) | ||
bucket.objects.filter(Prefix=(output.lower() + "-test/")).delete() | ||
|
||
|
||
def generate_daemonset_config(throughput): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider break this method into smaller one for readability line 532 - 609 , it too long to read :)
# Clear up testing resources | ||
- python ./load_tests/load_test.py delete_testing_resources | ||
- | | ||
for i in {1..2}; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add retries like this? IIRC I added retries in the python code already, which I think is better than using bash.
@@ -36,7 +36,7 @@ phases: | |||
fi | |||
|
|||
# Push the image to ECR | |||
- './scripts/publish.sh cicd-publish ${REGION_TO_PUSH}' | |||
- './scripts/publish.sh cicd-publish private-ecr' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why make this change?
@@ -11,7 +11,7 @@ phases: | |||
commands: | |||
# Enforce STS regional endpoints | |||
- export AWS_STS_REGIONAL_ENDPOINTS=regional | |||
- './scripts/publish.sh cicd-publish-ssm ${AWS_REGION}' | |||
- './scripts/publish.sh cicd-publish-ssm' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
This refactor allows for the sync tasks to be located on both production accounts and other accounts like the scanner account.
Also it serves as an abstraction for the CICD pipeline, to allow for the setup to be tested on test accounts.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.