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

Feature createorganizationalunit #693

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

ethanBaird
Copy link

Why?

In our existing Account Creation workflow, we are required to create new organizational units via the Organizations console in our master account. This requires us to sign in to our master account with heightened priveleges for every new account (that has a new path)

We run a multi account strategy so this happens reasonably often.

We'd prefer for this to be automated. This change will programmatically create any new Organizational Units if a new path is defined in the adf-accounts configuration.

Response to Issue: (#263)

What?

Description of changes:

  • Updated permissions on adf-codebuild-role in master account
  • Added a new method Organizations.create_ou()
  • Refactored existing method Organizations.get_ou_id()
    • refactor while loop to for loop
    • refactor logic in for/else to create OU if not found as defined by the path
  • stubbed tests for both methods

By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.

this is the service role for aws-deployment-framework-base-templates

this is the codebuild project where the provisioner runs which will be responsible for creating the new OUs
test for new method create_ou
test for ammended get_ou_id for/else
refactors while loop to for loop
changes logic in for/else to create ou if not found
Copy link
Contributor

@StewartW StewartW left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. A few nits and comments but otherwise a great addition.

The only question I have is if there should be a flag around this functionality to let customers keep the existing behavior.

@javydekoning
Copy link
Contributor

@ethanBaird , can you please fix these linter findings?

  src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/shared/python/organizations.py:
  	493: Trailing whitespace
  	497: Trailing whitespace
  	500: Trailing whitespace
  src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/shared/python/tests/stubs/stub_organizations.py:
  	Wrong line endings or no final newline

ethan-baird and others added 11 commits April 10, 2024 17:06
…ird/aws-deployment-framework into feature-createorganizationalunit
this is the service role for aws-deployment-framework-base-templates

this is the codebuild project where the provisioner runs which will be responsible for creating the new OUs
test for new method create_ou
test for ammended get_ou_id for/else
refactors while loop to for loop
changes logic in for/else to create ou if not found
@StewartW StewartW force-pushed the feature-createorganizationalunit branch from 955d3e4 to 8a59606 Compare April 19, 2024 09:25
@ethanBaird
Copy link
Author

Hey @StewartW 👋

Sorry, been a while since I've looked at this, but I have updated the create_ou method to throw an Organizations exception when encountering a boto ClientError and tested the method for this. 🚀

Regarding making this an optional flag in the adf_config.yml. I'm not sure what the best way to do this is.

I understand that we have the SSM Parameter config which we can read in. But should we be passing an ssm_client to every Organizations() object in order to read this?

Something like... in organizations.py

self.config = ssm_client.get_parameter('config')

Or I see we use the ParameterStore class in a few instances. Is it better to pass this to the Org Class and so something more like

self.config = parameter_store.fetch_parameter('config')

Let me know what you think.

@StewartW
Copy link
Contributor

self.config = parameter_store.fetch_parameter('config')

This is the better approach yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants