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

feat: Add --log functionality for additional commands and resources #13358

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

Conversation

cgetzen
Copy link
Contributor

@cgetzen cgetzen commented Oct 1, 2024

What this PR does / why we need it:

  • Jobs with the "helm.sh/hook": "test" annotation will now work with helm test --logs. This previously caused helm to error due to searching for a pod with the same name as the job.
  • Upgrades that have Pods or Jobs that contain an upgrade annotation (e.g. "helm.sh/hook": pre-upgrade) can now run with --logs to print the logs once the upgrade completes.

This is important because the cause of the hook related upgrade failures is not possible to determine with the helm output, and requires investigating kubernetes job logs.

Special notes for your reviewer:

I'm unsure if this code is possible to unit test because it relies on the output of annotated pods. If there is an integration test framework for helm, I will add tests to that. I was able to manually test with the following helm single-file chart:

apiVersion: batch/v1
kind: Job
metadata:
  name: post-install
  annotations:
    "helm.sh/hook": post-install
    "helm.sh/hook-delete-policy": before-hook-creation
spec:
  template:
    spec:
      containers:
      - name: test
        image: ubuntu
        command: ["echo",  "hello world"]
      restartPolicy: Never
  backoffLimit: 1
---
apiVersion: batch/v1
kind: Job
metadata:
  name: test
  annotations:
    "helm.sh/hook": test
spec:
  template:
    spec:
      containers:
      - name: test
        image: ubuntu
        command: ["echo",  "I'm a test!"]
      restartPolicy: Never
  backoffLimit: 1
---
apiVersion: v1
kind: Pod
metadata:
  name: backwards-compatible
  annotations:
    "helm.sh/hook": test
spec:
  containers:
  - name: test
    image: ubuntu
    command: ["echo",  "I'm a test pod!"]
  restartPolicy: Never

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 1, 2024
@cgetzen cgetzen force-pushed the feat-support-job-and-upgrade-hook-logs branch 3 times, most recently from 027db89 to b2158bc Compare October 1, 2024 07:24
Copy link
Contributor

@robertsirc robertsirc left a comment

Choose a reason for hiding this comment

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

The build-test and golinter actions are failing.

@robertsirc
Copy link
Contributor

Hello thank you for your PR. There are some issues with the GH actions could you please address that so we can review?

@cgetzen
Copy link
Contributor Author

cgetzen commented Oct 2, 2024

Hi @robertsirc, the latest commit looks successful on my end. If there's any links to failures I can take a look!

@robertsirc
Copy link
Contributor

We appreciate your PR. Your account is coming up Unverified could you correct that?

Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
@cgetzen cgetzen force-pushed the feat-support-job-and-upgrade-hook-logs branch from 0a8f877 to 707604c Compare October 2, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants