-
Notifications
You must be signed in to change notification settings - Fork 43
build-container-helm-chart-flux-demo #41
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
Conversation
8688d4f to
37c4554
Compare
06kellyjac
left a comment
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.
Good stuff
A few thoughts & suggestions
Also should we have a workflow that ensures the chart and/or the demo chart apply properly?
Can be in a future PR
| SnifferContainerImage: "docker.io/controlplane/netassertv2-packet-sniffer:v1.1.7", | ||
| SnifferContainerPrefix: "netassertv2-sniffer", | ||
| ScannerContainerImage: "docker.io/controlplane/netassertv2-l4-client:latest", | ||
| ScannerContainerImage: "docker.io/controlplane/netassertv2-l4-client:v1.0.6", |
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 we're committing this can we have a simple bash script or something to update this?
Also we could use digest sha
| spec: | ||
| initContainers: | ||
| - name: "sleepy" | ||
| image: busybox:1.36 |
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.
Probably best to use a full image path. I recall seeing a post about CRI-O requiring full path soon but I can't find the link
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.
Also if these images are defined in helm values then there's only 1 place to update them.
| spec: | ||
| containers: | ||
| - name: busybox | ||
| image: busybox |
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.
Same for this image
| command: | ||
| - sleep | ||
| - "360000" |
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.
Could probably follow the same pattern as above sleepy
| spec: | ||
| containers: | ||
| - name: webserver | ||
| image: nginx:latest |
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.
image path
|
|
||
| --- | ||
|
|
||
| ### 1.2 Create a Kind Cluster |
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.
| ### 1.2 Create a Kind Cluster | |
| ### 1.2 Create a KinD Cluster |
| - name: Push Helm chart to GHCR | ||
| run: | | ||
| CLEAN_VERSION=$(echo "$RELEASE_VERSION" | sed 's/^v//') | ||
| helm push "./netassert-${CLEAN_VERSION}.tgz" oci://ghcr.io/${{ github.repository_owner }}/charts |
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.
Missing newline EOF
| attestations: write | ||
| id-token: write | ||
| steps: | ||
| - name: Checkout repository |
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.
can some spacing be added between each step for readability please
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| packagerelease: |
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.
| packagerelease: | |
| helm-release: |
or
| packagerelease: | |
| container-and-helm: |
| build: | ||
| runs-on: ubuntu-latest | ||
| needs: lint | ||
| #needs: lint |
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.
Can we re-enable linting here or bring it back in a separate workflow?
cicci8ino
left a comment
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.
lgtm, with comments
No description provided.