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.

Sign up for GitHub

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

Jump to bottom

cloudtests: repatriate EnvironmentConfig (mz repo) #21349

Merged
merged 17 commits into from
Aug 28, 2023
Merged

cloudtests: repatriate EnvironmentConfig (mz repo) #21349

merged 17 commits into from
Aug 28, 2023

Conversation

nrainer-materialize
Copy link
Contributor

@nrainer-materialize nrainer-materialize commented Aug 23, 2023 •
edited
Loading

Motivation

The class EnvironmentConfig is quite repo-specific and should belong to the cloud repo to allow editing it without pain.

This PR refactors existing code:

  • it refactors util files to no longer require EnvironmentConfig
  • it removes the file EnvironmentConfig

Tips for reviewer

Commit-wise review suggested.

Checklist

  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • ⚠️ Do not merge before the cloud repo adaptation PR is ready.

@nrainer-materialize nrainer-materialize added the T-testing Theme: tests or test infrastructure label Aug 23, 2023
@nrainer-materialize nrainer-materialize self-assigned this Aug 23, 2023
@nrainer-materialize nrainer-materialize force-pushed the cloudtests/repatriate-envconfig branch 2 times, most recently from 9ff4bac to 2aee69e Compare August 23, 2023 12:47
rjobanp
rjobanp reviewed Aug 23, 2023
View reviewed changes
Copy link
Contributor

@rjobanp rjobanp left a comment

Choose a reason for hiding this comment

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

A couple of comments but looks good! thanks for taking on this work -- I know it's going to require some significant refactoring in the cloud repo :)

misc/python/materialize/cloudtest/util/environment.py Outdated Show resolved Hide resolved
misc/python/materialize/cloudtest/util/environment.py
response = get(
config,
config.controllers.region_api_server.configured_base_url(),
class Environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! I like encapsulating all these environment management calls under this class!

misc/python/materialize/cloudtest/util/authentication.py Outdated Show resolved Hide resolved
@nrainer-materialize nrainer-materialize force-pushed the cloudtests/repatriate-envconfig branch 2 times, most recently from 4955c70 to 4d3f6a0 Compare August 23, 2023 15:53
rjobanp
rjobanp reviewed Aug 23, 2023
View reviewed changes
Copy link
Contributor

@rjobanp rjobanp left a comment

Choose a reason for hiding this comment

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

I think this looks much better!

misc/python/materialize/cloudtest/util/authentication.py Show resolved Hide resolved
@nrainer-materialize
Copy link
Contributor Author

Rebased and added further commits after review:

eb0e7e0c0b cloudtests: add sugar to ControllerDefinition
aee4c0b328 cloudtests: move back docker_env as well
e49b0dd840 cloudtests: adapt web request usage
6fae2b7aa4 cloudtests: convert web-request utils to class
f68ae534a4 cloudtests: update setup.py
8103c56895 cloudtests: remove unnecessary params

@nrainer-materialize nrainer-materialize changed the title cloudtests: repatriate EnvironmentConfig cloudtests: repatriate EnvironmentConfig (mz repo) Aug 24, 2023
@nrainer-materialize nrainer-materialize force-pushed the cloudtests/repatriate-envconfig branch 4 times, most recently from 8a67cf2 to b6c4ec7 Compare August 28, 2023 16:32
@nrainer-materialize nrainer-materialize merged commit 003ae64 into MaterializeInc:main Aug 28, 2023
@nrainer-materialize nrainer-materialize deleted the cloudtests/repatriate-envconfig branch August 31, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjobanp rjobanp rjobanp approved these changes

@philip-stoev philip-stoev philip-stoev approved these changes

@necaris necaris necaris approved these changes

@evanharmon evanharmon Awaiting requested review from evanharmon evanharmon is a code owner automatically assigned from MaterializeInc/cloud

@alex-hunt-materialize alex-hunt-materialize Awaiting requested review from alex-hunt-materialize

@doy-materialize doy-materialize Awaiting requested review from doy-materialize

Assignees

@nrainer-materialize nrainer-materialize

Labels
T-testing Theme: tests or test infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nrainer-materialize @necaris @philip-stoev @rjobanp