-
Notifications
You must be signed in to change notification settings - Fork 0
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
Set cloud base url #20
Conversation
griptapecli/core/skatepark.py
Outdated
"GT_CLOUD_RUN_ID": run.run_id, | ||
"GT_CLOUD_BASE_URL": str(request.base_url), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the right ordering? Or should they be before os.environ
so they can be overridden by os envs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want customer to be able to override, so this'd be a gotcha, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm playing this back in my head, let me know if this is comprehensive and accurate:
- In Skatepark emulator, customer wants to override the base URL if they've selected a different URL or port. So having this up here and then overridden by os.environ makes sense.
- In Cloud, customer never wants to override the base URL, right? Or are there scenarios where they might?
- In all situations, what is the benefit to overriding the Run ID? Should that be explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think rather than try to predict when a user may or may not want to override these environment variables it's better to just have a consistent behavior for overrides.
- We provide some system level environment variables (
GT_CLOUD_RUN_ID
andGT_CLOUD_STRUCTURE_ID
). - Then
os.environ
is layered on. - Then
structure.env
is layered on. - Then
run.env
is layered on.
If you want to get fancy and override these, use at your own risk. If they try to set values for resources they don't own, auth will block them anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me. So what's the full set of changes we need to make to enshrine that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it works with the current changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the run ID is OK being set here, but I think we want the URL to be overridden by the os.environ, n'est-ce pas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking my logic for when someone would want to override and when it may be useless/harmful. Comment on thread plz.
griptapecli/core/skatepark.py
Outdated
"GT_CLOUD_RUN_ID": run.run_id, | ||
"GT_CLOUD_BASE_URL": str(request.base_url), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm playing this back in my head, let me know if this is comprehensive and accurate:
- In Skatepark emulator, customer wants to override the base URL if they've selected a different URL or port. So having this up here and then overridden by os.environ makes sense.
- In Cloud, customer never wants to override the base URL, right? Or are there scenarios where they might?
- In all situations, what is the benefit to overriding the Run ID? Should that be explicit?
32876c0
to
fc44a68
Compare
No description provided.