Supporting AWS Support Subscriptions #233
Conversation
|
What will happen when the bool is flipped from |
Hey, Yeah so the account would only be subscribed on creation. Updating the True|False value wouldn't have any impact. I guess this is keeping with the functionality already set by the delete_default_vpc functionality / tags i.e no revert function. |
| return True | ||
| except (ClientError, BotoCoreError) as e: | ||
| if e.response["Error"]["Code"] == "SubscriptionRequiredException": | ||
| LOGGER.warning("Failed subscription for account") |
There was a problem hiding this comment.
This warning can be removed, the info message is clear enough.
| LOGGER.warning("Failed subscription for account") | ||
| LOGGER.info('Enterprise Support is not enabled') | ||
| return False | ||
| raise e |
There was a problem hiding this comment.
Could you change this to become: raise?
There is a big difference between raise e and raise, where raise e generated a new stack trace instead of reusing the previous one. See this StackOverflow answer.
src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/provisioner/src/support.py
Show resolved
Hide resolved
src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/provisioner/src/support.py
Show resolved
Hide resolved
| except (ClientError, BotoCoreError) as e: | ||
| LOGGER.error('Failed to enable enterprise support for account: ' | ||
| f'{account.full_name} ({account.alias}): {account_id}') | ||
| LOGGER.error(e) |
There was a problem hiding this comment.
Could you please remove the last error and add exc_info=True as an argument in the first error log call.
This will enable the Python logger to capture the stacktrace.
You can add it by setting exc_info=True, like this:
LOGGER.error(
'Failed to enable enterprise support for account: '
f'{account.full_name} ({account.alias}): {account_id}',
exc_info=True,
)Additionally, I think we might want to raise the error just like you did in the check_aws_support_subscription function such that it will not go unnoticed.
| def __init__(self, role): | ||
| self.client = role.client("support", region_name='us-east-1', config=Support._config) | ||
|
|
||
| def check_aws_support_subscription(self): |
There was a problem hiding this comment.
Please add comments describing what the method does and what it returns, like this for example.
| return False | ||
| raise e | ||
|
|
||
| def enable_enterprise_support_for_account(self, account: Account, account_id): |
There was a problem hiding this comment.
Please add comments to describe the method, its inputs and what possible errors it might return.
|
@javydekoning Good point! I think we should update this in the documentation. So it clearly describes the to-be expected behavior for all of those attributes (only applies the value set at creation, not when changed after). |
There was a problem hiding this comment.
Thanks for the changes you made @StewartW.
Could you look into the last comments and fix those?
I would really like to add these changes to ADF.
Additionally, would you be able to merge the commits into one or two commits (i.e. rewrite history using git rebase -i)? You can find a good reference how to do this here [0].
Ensuring all changes that affect a single feature addition are in the same commit is especially helpful when debugging errors later :)
If you need support with the rebase feel free to ask for help.
Many thanks!
src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/provisioner/main.py
Outdated
Show resolved
Hide resolved
src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/provisioner/main.py
Show resolved
Hide resolved
StewartW
force-pushed
the
enable-enterprise-support-on-creation
branch
from
5fff1b0 to
471929e
Compare
There was a problem hiding this comment.
Your changes look great, thanks for developing this feature!
One last thing: it seems that the double newline is still part of the last commit you pushed.
Could you remove that one so we can get it merged into ADF?
Removing .idea folder Fixing linter errors Ignoring Pycharm files Implementing Code Review Comments Removing references to enterprise support, making the process a bit more generic going forward. Making it run on every execution rather than just on account creation, adding on some logic for detecting desired support type and when to do ticket creation. Also updated logging throughout to be a bit more verbose to assist debugging future issues. Changed Support Subscription to run on account creation Updating documentation Adding in the exact usecase, with emphasis
StewartW
force-pushed
the
enable-enterprise-support-on-creation
branch
from
471929e to
f20fac3
Compare
Apologies for the delay in fixing that. Amended the commit now. |
There was a problem hiding this comment.
LGTM: Would be great to get this added!
@thomasmcgannon could you review this PR?
|
Hey @sbkok / @deltagarrett any ETA on if we can get this merged? |
**Features 🏗** * Adds Enterprise Support to account creation process awslabs#233, closes awslabs#232: * ADF will raise a ticket to add the account to an existing AWS support subscription when an account is created. As a prerequisite, your organization master account must already have enterprise support activated. * Adds nested deployment map support awslabs#266 and awslabs#328, closes awslabs#265: * This enables usage of sub directories within the deployment_maps folder. **Fixes 🐞** * Fixes specific role usage to be used in Build and Deploy only awslabs#295. * Corrects removing pipelines anchor in docs awslabs#279. * Fixes CI builds due to isort version mismatch awslabs#284. * Fixes error handling of generate_params intrinsic upload function awslabs#277, closes awslabs#276. * Fixes spec_inline attribute of CodeBuild in docs awslabs#289. * Fixes provider spec_inline support of CodeBuild in awslabs#293. * Fixes supported list of intrinsic upload path styles, enables usage of s3-url and s3-key-only awslabs#275, closes awslabs#299. * Fixes create deployment account concurrency failure awslabs#287, closes awslabs#280. * Fixes approval stage usage, by limiting specific role usage to Build and Deploy steps awslabs#295. * Fixes yarnpkg GPG awslabs#313, closes awslabs#325. * Removes dependency on botocore.vendored.requests awslabs#326, closes awslabs#324. **Improvements ✨** * Improves docs on providers and their properties awslabs#274. * Separates pipeline cleanup from input generation script awslabs#288. * Upgrades Python from v3.7 to v3.8 awslabs#313. * Upgrades CodeBuild image from "aws/codebuild/standard:2.0" to "aws/codebuild/standard:5.0" awslabs#313, closes awslabs#267, closes awslabs#300. * Upgrades CDK from v1.32 to v1.88 awslabs#313, closes awslabs#292.
Issue #, if available:
resolves issue #232
Description of changes:
Added
enable_enterprise_supportfor Account schema (Defaults to False, if not given)Modified
comments:
Unsure about the location of the support class. I was thinking it should probably be the shared/ directory, but it imports the Account class from the provisioner source code, so I've placed it there. Let me know what you think about this.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.