-
Notifications
You must be signed in to change notification settings - Fork 7
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
use USER env and create svc with yaml instead of expose + other changes #526
Conversation
PR environments: |
with curl instead of browser & change local deploy yaml to match for replicas
@@ -25,28 +25,23 @@ In a sidecar pattern, the functionality of the main container is extended or enh | |||
In {{<link "persistent-storage">}} you created a MariaDB deployment. In this task you are going to add the [Prometheus MySQL exporter](https://github.com/prometheus/mysqld_exporter) to it. | |||
|
|||
{{% onlyWhenNot openshift %}} |
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.
shall I change this also for OpenShift?
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.
What exactly?
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.
Ah, in the overview it didn't show the following changes. We can also change this in OpenShift, that's ok, thanks.
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.
And this is also changed in the oc variants
6cf9b35
to
59c5290
Compare
4466b0c
to
6cac131
Compare
Why use Or even better, use placeholders that can be set via URL parameters instead of letting the user set env vars. The reason for this is, depending on the training, we have different naming conventions, especially if we are on customer-provided infrastructure. |
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 couldn't yet review all of it and will certainly have to test all the changes in more detail, but these are my first suggestions.
|
||
```bash | ||
{{% param cliToolName %}} expose deployment example-web-go --type=ClusterIP --name=example-web-go --port=5000 --target-port=5000 --namespace <namespace> |
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.
In my opinion it's important to know these helper commands as they facilitate editing resources and belong to a basics training. At a later time the resources are then saved as files in order to adopt a gitops approach.
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.
If I remember correctly we once discussed, that we wan't to get rid of these imperative commands. Not sure if you were involved in this discussion though. In my opinion the --dry-run=client -o yaml
approach is the nicer one to create resources for the gitops approach. But I guess for expose
there is not relly an equivalent.
Maybee also something to quickly discuss/align in a S1 Stream meeting.
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 could also use oc create route
. However, this only works for secured routes, so we'd need to make sure all OpenShift training variants are run on a cluster that offers a proper wildcard certificate or cert-manager that handles these. Up until now this is the case.
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 added the expose
command again as a second option.
@schlapzz (acend/helm-basics-training#395) already used this in the helm training, so I reused it from there. Also |
2b0e242
to
b1e2fe0
Compare
b1e2fe0
to
7a00943
Compare
As Seba already mentioned, we're make heavy use of this Env Var in our Trainings ($LAB_USER in AMM Techlab, later renamed in the Helm & ArgoCD Techlab from $STUDENT to $USER, see here and later here The naming of the var is not that important for me. But regarding the new Training Cluster setup it would be nice if we use the same Var Name for all trainings 💙 |
Still, the best option imho would be to use a placeholder as described here. This doesn't require setting an environment variable on the terminal and is widely used in the Kubernetes Basics training. |
I replaced |
e07e571
to
dc70d14
Compare
No description provided.