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

[RPD-259] Refactor build_state_from_terraform_output within matcha_state.py to use objects defined within matcha_state.py #190

Merged

Conversation

swells2020
Copy link
Contributor

@swells2020 swells2020 commented Aug 9, 2023

This PR refactors the build_state_from_terraform_output method to make use of the custom classes within the matcha_state.py module.

Previously, the build_state_from_terraform_output method made use of the _parse_terraform_output utility function to build a dictionary representation of state. This dictionary was then converted into a MatchaState object using the from_dict method in the MatchaState class.

In this new implementation, the MatchaState object is built using the relevant classes representing underlying resources, properties and components.

This PR also updates the get_component method of the MatchaStateService by moving it to the MatchaState object and adding an ability for the method to return None. Changes have been made to the broader matcha codebase to account for this change.

Checklist

Please ensure you have done the following:

  • I have read the CONTRIBUTING guide.
  • I have updated the documentation if required.
  • I have added tests which cover my changes.

Type of change

Tick all those that apply:

  • Bug Fix (non-breaking change, fixing an issue)
  • New feature (non-breaking change to add functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@swells2020 swells2020 changed the base branch from develop to fixes/RPD-278 August 9, 2023 14:49
@swells2020 swells2020 self-assigned this Aug 10, 2023
@swells2020 swells2020 changed the title [WIP] Rpd 259 refactor build state from terraform output [RPD-259] Refactor build_state_from_terraform_output within matcha_state.py to use objects defined within matcha_state.py Aug 10, 2023
Copy link
Member

@Christopher-Norman Christopher-Norman left a comment

Choose a reason for hiding this comment

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

Good so far, added a couple of comments.

src/matcha_ml/services/analytics_service.py Outdated Show resolved Hide resolved
src/matcha_ml/state/matcha_state.py Outdated Show resolved Hide resolved
src/matcha_ml/state/matcha_state.py Outdated Show resolved Hide resolved
src/matcha_ml/state/matcha_state.py Show resolved Hide resolved
Copy link
Contributor

@KirsoppJ KirsoppJ left a comment

Choose a reason for hiding this comment

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

LGTM besides the stuff Chris already commented on and one nit


component = matcha_state.get_component(resource_type.name)

if component:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be a bit easier to read if this was an if component is not None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks.

Copy link
Member

@Christopher-Norman Christopher-Norman left a comment

Choose a reason for hiding this comment

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

LGTM

@swells2020 swells2020 merged commit bf43702 into fixes/RPD-278 Aug 11, 2023
2 checks passed
@JonoCX JonoCX mentioned this pull request Aug 14, 2023
7 tasks
JonoCX added a commit that referenced this pull request Aug 15, 2023
* [RPD-287] ZenML version inference for zenserver (#180)

* various changes

* change variable names to make things more clear

* shubham's comment

* test for latest

* update existing tests

* update existing tests

* shubham latest comment

* add gitflow notation branches to ci (#183)

* remove old, commented out code

* [RPD-260] Add an object to handle the `matcha.config.json` file. (#184)

* adds logic and tests for matcha config module

* updates docstrings

* adds tests and implements config object throughout matcha

* updated for pr comments

* updates docstring

* fixes ci

* updates for comments

* [RPD-249] Refactor `analytics_service` to simplify tracking decorator (#181)

* refactor analytics service

* adds tests and updates for comments

* updates tests

* updates for comments

* extracts _get_state_uuid logic into private function

* removes old _get_state_uuid logic

* updates _execute_analytics_event_function

* extracts _time_event logic

* adds _post_event logic

* adds tests

* adds dataclass

* updates tests

* [RPD-236] Improved approach to check ignored folder when uploading and downloading (#186)

* [RPD-250] Move `_show_terraform_outputs()` into `provision` (#188)

* updates core and azure_runner

* adds test

* updated for comments

* updates test fixtures

* [RPD-259] Refactor `build_state_from_terraform_output` within `matcha_state.py` to use objects defined within matcha_state.py (#190)

* updates build_state_from_terraform_outputs

* updates for comments

* fixes tests

* removes print statement

* updates get_component

* updates get_component

* updates for comments

* [RPD-290] Update MatchaConfig object to not throw an error when a matching property/component is not found (#192)

---------

Co-authored-by: KirsoppJ <40233184+KirsoppJ@users.noreply.github.com>
Co-authored-by: Callum Wells <68609181+swells2020@users.noreply.github.com>
@Christopher-Norman Christopher-Norman deleted the RPD-259-refactor-build_state_from_terraform_output branch August 23, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants