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

Close quotes on kubernetes_context values in gcp-modular #100

Merged

Conversation

cameronraysmith
Copy link
Contributor

@cameronraysmith cameronraysmith commented Sep 25, 2023

Failure to close quotations in the stack_file kubernetes_context values leads to errors like

Importing stack 'mlops' into ZenML...
...
ParserError: while parsing a flow mapping
  in "<unicode string>", line 27, column 24:
            configuration: {"kubernetes_context": "gke_zenm ... 
                           ^
expected ',' or '}', but got '<scalar>'
  in "<unicode string>", line 27, column 75:
     ... t": "gke_zenml-cluster-6i6x, "synchronous": True}
                                         ^

when running commands such as

mlstacks deploy -f stack.yaml -d
zenml stack deploy -p gcp -n mlops -r us-east4 -f stack.yaml -d

in zenml v0.44.2 and mlstacks v0.7.5.

@cameronraysmith cameronraysmith marked this pull request as ready for review September 25, 2023 18:34
@strickvl strickvl self-requested a review September 26, 2023 07:17
@strickvl
Copy link
Contributor

Thanks for catching this @cameronraysmith! We take PRs on the develop branch, so could you possibly rebase this PR / branch on develop instead of making it point to main? https://github.com/zenml-io/zenml/blob/main/CONTRIBUTING.md#-pull-requests-rebase-your-branch-on-develop has instructions in case you need / want that (taken from the sister core zenml repository).

@cameronraysmith cameronraysmith changed the base branch from main to develop September 26, 2023 13:46
@cameronraysmith cameronraysmith force-pushed the fix-kubernetes-context-quotation branch from c96b7e5 to 5948987 Compare September 26, 2023 13:49
@cameronraysmith
Copy link
Contributor Author

done.

Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

LGTM, pending tests passing. Thanks for the contribution!

@strickvl strickvl added the bug Something isn't working label Sep 26, 2023
@strickvl strickvl changed the title fix: close quotes on kubernetes_context values Close quotes on kubernetes_context values in gcp-modular Sep 26, 2023
@cameronraysmith cameronraysmith force-pushed the fix-kubernetes-context-quotation branch 2 times, most recently from 5df6d10 to 1dae461 Compare September 26, 2023 14:29
@cameronraysmith cameronraysmith force-pushed the fix-kubernetes-context-quotation branch from 1dae461 to 1b606fe Compare September 26, 2023 14:30
@cameronraysmith
Copy link
Contributor Author

Let me know if it needs to be rebased again.

@strickvl
Copy link
Contributor

@cameronraysmith yeah actually not sure what the issue is... the error message seems to be unrelated to the changes + develop branch itself doesn't have issues...

@cameronraysmith
Copy link
Contributor Author

cameronraysmith commented Sep 26, 2023

What is the error message? It's rebased on develop up to 71b97b5

1b606fe (HEAD -> fix-kubernetes-context-quotation, origin/fix-kubernetes-context-quotation) fix: close quotes on kubernetes_context values
71b97b5 (upstream/develop, develop) Add `skypilot` support for AWS and GCP (#99)
5069e56 update PR template to discourage PRs on main (#101)
64ce5c9 Add remote state deployment + use by default (#97)

@strickvl
Copy link
Contributor

I see what's going on -- here. It's not passing the internal secrets down since you're contributing from a forked branch, so that's why the tests don't work. In any case this is a tiny PR with only fixes, no breaking changes, so I'll merge it into develop once the rest of the tests pass. Thanks again!

@cameronraysmith
Copy link
Contributor Author

cameronraysmith commented Sep 26, 2023

Some of these jobs like aws_test and gcp_test failed in #99 here https://github.com/zenml-io/mlstacks/actions/runs/6310939091 so they aren't going to pass after rebasing on it 71b97b5. Since you already merged that one there shouldn't be an additional issue with this one.

It's not passing the internal secrets down since you're contributing from a forked branch, so that's why the tests don't work.

I see. You are correct.

@cameronraysmith
Copy link
Contributor Author

cameronraysmith commented Sep 26, 2023

Thank you @strickvl. Apologies for the confusion!
It doesn't look like there's an option to allow usage of secrets even when maintainers approve workflow runs.

@strickvl strickvl merged commit f0604fb into zenml-io:develop Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants