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

Fix generate_params.py to rely on pipeline input definitions instead #559

Merged
merged 2 commits into from Dec 7, 2022

Conversation

sbkok
Copy link
Collaborator

@sbkok sbkok commented Dec 7, 2022

Why?

The generate_params.py script relied on the account_ous.json file that was uploaded to the shared modules bucket.

While ADF got support for newer account target selection mechanisms, the account_ous.json file structure was not updated accordingly.

To ensure future support and ease to maintain new account target selections it was decided to drop the account_ous.json and move to the pipeline input definition instead.

What?

  • Updated generate_params.py to rely on the pipeline input definition instead.
  • Updated generate_params.py to ensure that parameters would be generated correctly. Refactored it, as before it was using a complex structure inside the resolver class. With the new changes, it relies on generating a new copy of the parameters with each step instead. Making it easier to test and harder to break.
  • Refactored the resolver class to a version that is easier to understand and easy to extend using resolver classes.
  • Added multiple test cases to ensure correctness of the generate_params.py script.
  • The shared modules bucket was used to share the accounts and ous from the pipeline definition to the pipeline itself before. With this change, it is using the pipeline definition bucket and both the pipeline definition bucket and the shared modules bucket are now read only for the pipeline itself.
  • There was another bucket accessible to the pipeline, but that was not used, hence dropping access to it.

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

**Why?**

The `generate_params.py` script relied on the `account_ous.json` file that was
uploaded to the shared modules bucket.

While ADF got support for newer account target selection mechanisms, the
`account_ous.json` file structure was not updated accordingly.

To ensure future support and ease to maintain new account target selections
it was decided to drop the `account_ous.json` and move to the pipeline input
definition instead.

**What?**

* Updated generate_params.py to rely on the pipeline input definition instead.
* Updated generate_params.py to ensure that parameters would be generated
  correctly. Before it was using a complex structure inside the resolver class.
  With the new changes, it relies on generating a new copy of the parameters
  with each step instead. Making it easier to test and harder to break.
* Updated the resolver class to a simplified version using easy to extend
  resolver classes.
* Added multiple test cases to ensure correctness of the generate_params.py
  script.
@sbkok sbkok added this to the v3.2.0 milestone Dec 7, 2022
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.

Looks great!

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.

None yet

3 participants