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 clearer instructions for kapp-controller in openshift+gcp #480

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

joaopapereira
Copy link
Member

No description provided.

variable](/imgpkg/docs/latest/auth/#via-iaas) to false on the sidecar container.
Next is an example of the change needed based on the [release yaml](https://github.com/vmware-tanzu/carvel-kapp-controller/releases/download/v0.38.4/release.yml)
```
index 806dfab..895cd48 100644
Copy link
Contributor

Choose a reason for hiding this comment

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

just my $0.02: no need for this line with the hashes in it as it will be different for everyone

Copy link
Member Author

Choose a reason for hiding this comment

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

the SHA's are not relevant because I made this diff outside a repository. But I can remove them

value: /etc/kappctrl-mem-tmp/sidecarexec.sock
+ - name: IMGPKG_ENABLE_IAAS_AUTH
+ value: false
image: ghcr.io/vmware-tanzu/carvel-kapp-controller@sha256:fb1345342c98fb1f88ffdc3a4ec35228404abff0c528fabf7bead33a2564854c
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess technically for versions that have been released this SHA will never change but i'd be tempted to truncate it or something in order to indicate that it will not change (ie it would be a mistake to copy-paste this line in a few months, because you almost surely want a different SHA here)

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem here is the diff needs a reference and that is the first line after the env....

Copy link
Contributor

@aaronshurley aaronshurley Jul 12, 2022

Choose a reason for hiding this comment

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

Thinking out loud... How important is the diff info? I'm not sure if it adds value for me. In the spirit of simplification and preventing misuse, would it be simpler if we provided just enough info for users to add this value themselves? A couple more options come to mind:

Option A: snippet of release.yml with just enough structural information

---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: kapp-controller
spec:
  template:
    spec:
      containers:
        - name: kapp-controller-sidecarexec
          env:
          - name: IMGPKG_ENABLE_IAAS_AUTH # add this name and value
            value: false

Option B: provide a mapping to the property

  1. Open the release yaml
  2. Navigate to the kapp-controller Deployment
  3. Add the environment variable to spec.template.spec.containers[name: kapp-controller-sidecarexec].env:
    - name: IMGPKG_ENABLE_IAAS_AUTH # add this name and value
      value: false
    

Note: I suspect that containers[name: kapp-controller-sidecarexec] is invalid but I hope it demonstrates the idea. Perhaps we'd find the "right" way to map it following jq's syntax or something.

Just sharing some ideas, don't consider this comment blocking.

Copy link
Contributor

@aaronshurley aaronshurley left a comment

Choose a reason for hiding this comment

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

I shared my thoughts but my comments are not blocking.

+ value: false
image: ghcr.io/vmware-tanzu/carvel-kapp-controller@sha256:fb1345342c98fb1f88ffdc3a4ec35228404abff0c528fabf7bead33a2564854c
name: kapp-controller-sidecarexec
resources:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could remove the resources: line as it doesn't add anything to the snippet.

value: /etc/kappctrl-mem-tmp/sidecarexec.sock
+ - name: IMGPKG_ENABLE_IAAS_AUTH
+ value: false
image: ghcr.io/vmware-tanzu/carvel-kapp-controller@sha256:fb1345342c98fb1f88ffdc3a4ec35228404abff0c528fabf7bead33a2564854c
Copy link
Contributor

@aaronshurley aaronshurley Jul 12, 2022

Choose a reason for hiding this comment

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

Thinking out loud... How important is the diff info? I'm not sure if it adds value for me. In the spirit of simplification and preventing misuse, would it be simpler if we provided just enough info for users to add this value themselves? A couple more options come to mind:

Option A: snippet of release.yml with just enough structural information

---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: kapp-controller
spec:
  template:
    spec:
      containers:
        - name: kapp-controller-sidecarexec
          env:
          - name: IMGPKG_ENABLE_IAAS_AUTH # add this name and value
            value: false

Option B: provide a mapping to the property

  1. Open the release yaml
  2. Navigate to the kapp-controller Deployment
  3. Add the environment variable to spec.template.spec.containers[name: kapp-controller-sidecarexec].env:
    - name: IMGPKG_ENABLE_IAAS_AUTH # add this name and value
      value: false
    

Note: I suspect that containers[name: kapp-controller-sidecarexec] is invalid but I hope it demonstrates the idea. Perhaps we'd find the "right" way to map it following jq's syntax or something.

Just sharing some ideas, don't consider this comment blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants