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

Refactor environment build() method #398

Merged
merged 10 commits into from
Dec 12, 2018
Merged

Refactor environment build() method #398

merged 10 commits into from
Dec 12, 2018

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Dec 12, 2018

This PR makes two principal changes:

  • Environments are immutable objects
  • Environments return JSON keys when built, not strings

Previously, environments were a little complex and mutated themselves when they were built, which had the potential to lead to statefulness bugs (if an environment were built twice, for example). Now, they are purely functional classes that are instantiated with any known values and produce dynamic values as a key when their build() method is called. In order to run the environment, the dynamic key is passed to the run() method, where it is combined with static attributes to manipulate the flow.

Note to @joshmeek this will slightly affect the GQL call that the deploy agent needs to make to collect the data it requires

@jlowin jlowin changed the title Refactor environment builds Refactor environment build() method Dec 12, 2018
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

A few minor comments; for the comment concerning the use of the term "key", just a docstring update is all I was thinking. I see that the word "key" is used all over the place.

Also your 3.4 and 3.5 tests appear to be failing due to an ordering issue.

cicdw
cicdw previously approved these changes Dec 12, 2018
- registry_url (str, optional): The registry to push the image to
- python_dependencies (list, optional): The list of pip installable python packages
that will be installed on build of the Docker container
- secrets (list, optional): A list of secret value names to be loaded into the environment
- flow_id (str, optional): A consistent flow ID (generally set on build and not passed in)
"""

def __init__(
self,
image: str,

Choose a reason for hiding this comment

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

Can this change to base_image?

@jlowin jlowin changed the base branch from master to feature/envs December 12, 2018 14:54
@jlowin jlowin closed this Dec 12, 2018
@jlowin jlowin changed the base branch from feature/envs to master December 12, 2018 15:01
@jlowin jlowin reopened this Dec 12, 2018
@jlowin jlowin changed the title Refactor environment build() method [DONT MERGE YET] Refactor environment build() method Dec 12, 2018
@jlowin jlowin added enhancement An improvement of an existing feature DONT MERGE This PR shouldn't be merged (yet) labels Dec 12, 2018
@jlowin
Copy link
Member Author

jlowin commented Dec 12, 2018

Marking as DON'T MERGE YET just as a precaution because there are going to be some incoming dependent PRs, and also this PR will require some minor tweaks to the deploy agent.

@jlowin jlowin changed the title [DONT MERGE YET] Refactor environment build() method Refactor environment build() method Dec 12, 2018
@jlowin jlowin mentioned this pull request Dec 12, 2018
@jlowin jlowin removed the DONT MERGE This PR shouldn't be merged (yet) label Dec 12, 2018
@jlowin jlowin merged commit 07affdd into master Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants