-
Notifications
You must be signed in to change notification settings - Fork 51
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
update code for better use, addind delay and additional condition #150
base: master
Are you sure you want to change the base?
Conversation
status = json.loads(st_call.data.decode('utf-8'))['status'] | ||
print("Status: " + status) | ||
while status == 'creating': | ||
st_call = http.request('GET', url , headers = {'Authorization': cloud_one_api_key, 'Api-Version': 'v1'}) |
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.
This part could also use the headers
variable you declared as well.
st_call = http.request('GET', url , headers = {'Authorization': cloud_one_api_key, 'Api-Version': 'v1'}) | |
st_call = http.request('GET', url, headers=headers) |
@@ -310,10 +319,11 @@ Resources: | |||
check_status(cloud_one_api_key, url) | |||
#check storage stack status | |||
def check_status(cloud_one_api_key, url): | |||
time.sleep(10) #Adding delay to avoid response error in API |
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.
Could you explain a bit more about the response error mentioned here? and why you choose to use 10 seconds as the delay interval?
else: | ||
add_tag(s3_client, bucket_name, tag_list=tag_status) #move this inside if to avoid duplicate tags | ||
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) |
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.
By moving the add_tag()
and add_storage()
inside the for loop means that every tag inside tag_status
array that has a key != "FSSMonitored"
will trigger the add_tag()
and add_storage()
once. I don't think this is what we wanted.
else: | |
add_tag(s3_client, bucket_name, tag_list=tag_status) #move this inside if to avoid duplicate tags | |
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) | |
add_tag(s3_client, bucket_name, tag_list=tag_status) #move this inside if to avoid duplicate tags | |
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) |
elif tags["Value"].lower() != "yes": | ||
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) | ||
break |
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.
Just curious, if the user gave an invalid FSSMonitored
tag value like FSSMonitored: heehee
, this plugin would still deploy the storage stack, right? In this case, should we also update their invalid FSSMonitored
value to Yes
instead of leaving it as heehee
?
for tags in tag_status: | ||
if tags["Key"] == "FSSMonitored": | ||
print(tags["Value"].lower()) |
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.
Don't think we need this since the tag set logs will print out the detailed tag structure and values already.
add_tag(s3_client, bucket_name, tag_list=[]) | ||
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) | ||
else: | ||
print("tag_Status2: ", tag_status) |
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.
Since we want to check the updated bucket tag after new tags were added, maybe consider adding the log at the end of add_tag()
instead?
def add_tag(s3_client, bucket_name, tag_list):
tag_list.append({'Key':'FSSMonitored', 'Value': 'Yes'})
print(f"Bucket: {bucket_name} lacks an FSSMonitored tag; adding")
s3_client.put_bucket_tagging(
Bucket=bucket_name,
Tagging={"TagSet": tag_list},
)
print("Updated bucket tag set: ", tag_list)
except ClientError: | ||
no_tags = "does not have tags" | ||
tag_status = no_tags | ||
if tag_status == "does not have tags": | ||
print("does not have tags") | ||
add_tag(s3_client, bucket_name, tag_list=[]) | ||
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) | ||
else: |
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.
We can simplify the code by taking out the if condition when tag_status == "does not have tags"
and just directly execute the actions inside the exception section like so:
except ClientError: | |
no_tags = "does not have tags" | |
tag_status = no_tags | |
if tag_status == "does not have tags": | |
print("does not have tags") | |
add_tag(s3_client, bucket_name, tag_list=[]) | |
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) | |
else: | |
except ClientError: | |
add_tag(s3_client, bucket_name, tag_list=[]) | |
add_storage(cloud_one_api_key, bucket_name, ext_id, account_id, stack_id, kms_arn) | |
return 0 |
tag_status = tags | ||
print("tag_Status1: ", tag_status) |
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.
Maybe consider logging sentences that are more readable:
tag_status = tags | |
print("tag_Status1: ", tag_status) | |
print("Bucket tag set: ", tags) |
Update Code
Change Summary
Adding delay in Check_Status to avoid response error on FSS Api
Adding 1 condition to handle the tags of S3 Buckets
Setting else condition to avoid duplicate tags for FSSMonited on add_tags method
PR Checklist
Other Notes
Duplicate tags
![image](https://private-user-images.githubusercontent.com/7684993/268870413-c79d1c19-4bf0-40d5-9fc9-1ad280838b0e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMTQxNDYsIm5iZiI6MTcyNDExMzg0NiwicGF0aCI6Ii83Njg0OTkzLzI2ODg3MDQxMy1jNzlkMWMxOS00YmYwLTQwZDUtOWZjOS0xYWQyODA4MzhiMGUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDgyMCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA4MjBUMDAzMDQ2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YzlkMzgwYjY4NDU1ODZlYzE0MmViNGMwNjEyNTNlNzE3Y2Q1M2Q1MDdlOTBiYjlmMGRkMGIwMjEzNzYwNTYyMyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.4O3G1NDJrPs4ZrvLDIXzNaFy41n3mGuUMzRvTdz3Z3g)
API error response
![image](https://private-user-images.githubusercontent.com/7684993/268870722-3d2bc786-f641-444a-9aec-0fe3f9176ef8.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMTQxNDYsIm5iZiI6MTcyNDExMzg0NiwicGF0aCI6Ii83Njg0OTkzLzI2ODg3MDcyMi0zZDJiYzc4Ni1mNjQxLTQ0NGEtOWFlYy0wZmUzZjkxNzZlZjgucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDgyMCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA4MjBUMDAzMDQ2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9N2VkZGVmODVhNzBmNmUyNTA0M2U3ZjY0MGVlNjA4OTJlMTliODA1MTJlNjhkOWZlM2QxNjNlYWMwMjNhN2E3MyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.eNl6tkQypakryRii-R4dQk90mVHgIzn7XSumu5-FAPM)