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

Add metadata.namespace to K8s YAML files #133

Merged
merged 5 commits into from
Aug 12, 2024
Merged

Add metadata.namespace to K8s YAML files #133

merged 5 commits into from
Aug 12, 2024

Conversation

jason-seqera
Copy link
Contributor

This PR adds namespace: seqera-nf to the K8s YAML files.

@jason-seqera jason-seqera self-assigned this Jul 24, 2024
Copy link

netlify bot commented Jul 24, 2024

Deploy Preview for seqera-docs ready!

Name Link
🔨 Latest commit 34fd52c
🔍 Latest deploy log https://app.netlify.com/sites/seqera-docs/deploys/66b8bf0f5742400007e8bf52
😎 Deploy Preview https://deploy-preview-133--seqera-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pditommaso
Copy link
Contributor

Why?

@jason-seqera
Copy link
Contributor Author

@gwright99 requested this: See https://seqera.atlassian.net/browse/EDU-126

@pditommaso
Copy link
Contributor

I see, but this would lock down the namespace name in the manifest file. As a customer, I want to be able to use my own namespace. This means those manifests will not be usable anymore to any customer (without prior editing)

@jason-seqera
Copy link
Contributor Author

@pditommaso, completely agree.

@gwright99
Copy link

IMO, it's better to have the namespace explicitly defined in the manifests. This makes the key explicit, does not require the operator to remember to switch their kubectl namespace context, and is easily fixed with a simple IDE find/replace action (I did this with a client just a few days ago -- it took 5 seconds to swap in their desired namespace).

With that said, this is not a hill I'm willing to die on. If there's opposition to the change, just leave it as is and push more cognitive load back onto the user deploying. The first K8s Installation step in the docs says to create a namespace, so maybe that is sufficient in itself.

@pditommaso
Copy link
Contributor

I leave it to you if keeping or no.

My tip if keeping it:

  1. use a self-descriptive, easy-to-replace identifier, e.g. platform-namespace
  2. Highlight in the documentation that the namespace should be replaced with the customer's one.
  3. it may be considered a breaking change

@jason-seqera
Copy link
Contributor Author

I've looked and looked, but manually setting the namespace in K8s resources is not standard practice that I can see. Splitting the difference here makes it more work for K8s beginners and experts alike. Either switching to the target namespace, as we have in the docs, or executing kubectl apply -f <file>.yml -n <target_namespace>, is the most common practice, and what the Kubernetes official documentation suggestions.

Could elaborate in Create a namespace on why the namespace matters and how to confirm that you're in the correct namespace before starting, and that if you don't specify, the default namespace is used by K8s.

@justinegeffen justinegeffen self-requested a review July 29, 2024 06:29
@justinegeffen
Copy link
Contributor

I leave it to you if keeping or no.

My tip if keeping it:

  1. use a self-descriptive, easy-to-replace identifier, e.g. platform-namespace
  2. Highlight in the documentation that the namespace should be replaced with the customer's one.
  3. it may be considered a breaking change

I really like this suggestion. For point 2, I would add a sentence to the line about creating a namespace that indicates "The manifests provided include the identifier platform-namespace which you can replace with your own namespace."

@pditommaso, would you say this is a breaking change for customers who are revisiting this page and copying the manifest which now would include an additional field? That definitely is a concern and if this is a regular occurrence I would add an admonition that if an error is encountered, to check the namespace.

@pditommaso
Copy link
Contributor

Not sure it should be classified as breaking change, the important this is highlighting the need to change the namespace with a real one

@jason-seqera
Copy link
Contributor Author

So regardless, a user still has to create the secret in the correct namespace, which is the first step. Creating a namespace and switching to that namespace context.

1. Create a [secret][kubectl-secret]:

    ```bash
    kubectl create secret docker-registry cr.seqera.io \
      --docker-server=cr.seqera.io \
      --docker-username='<YOUR NAME>' \
      --docker-password='<YOUR SECRET>'
    ```

For every other step, five separate section steps, we need to remind the user to replace <seqera_namespace> in the manifests with the proper namespace to use, for the context that they're already in because they created a namespace and then created the required secret in it.

I think this is adding unnecessary to complexity to the Kubernetes deployment.

@justinegeffen
Copy link
Contributor

So regardless, a user still has to create the secret in the correct namespace, which is the first step. Creating a namespace and switching to that namespace context.

1. Create a [secret][kubectl-secret]:

    ```bash
    kubectl create secret docker-registry cr.seqera.io \
      --docker-server=cr.seqera.io \
      --docker-username='<YOUR NAME>' \
      --docker-password='<YOUR SECRET>'
    ```

For every other step, five separate section steps, we need to remind the user to replace <seqera_namespace> in the manifests with the proper namespace to use, for the context that they're already in because they created a namespace and then created the required secret in it.

I think this is adding unnecessary to complexity to the Kubernetes deployment.

This is a reasonable point. If it's not standard practice and will bloat our docs - and lead to customer confusion - I would recommend not moving ahead with this change. It's definitely something we can keep an eye on to ensure we adjust if needed.

@jason-seqera
Copy link
Contributor Author

@pditommaso, I've revised this to clarify the purpose of the namespace. If this looks okay, I can merge it. Thanks!

justinegeffen and others added 4 commits August 11, 2024 15:35
@justinegeffen
Copy link
Contributor

Fixed some typos, renamed the namespaces mentioned, and resolved conflicts.

This is ready to merge. Thank you @pditommaso for your input!

@justinegeffen justinegeffen added the 3. Reviews complete Reviews complete. Remove label when confirmed in prod. label Aug 11, 2024
@jason-seqera jason-seqera merged commit 4b48486 into master Aug 12, 2024
6 checks passed
@jason-seqera jason-seqera deleted the EDU-126 branch August 12, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. Reviews complete Reviews complete. Remove label when confirmed in prod.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants