-
Notifications
You must be signed in to change notification settings - Fork 4
feat: wg-easy add preflights #59
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
base: main
Are you sure you want to change the base?
Conversation
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 was trying to do a review and then I realized you've based this on another active development branch but targeted the PR against main.
The problem is that means I'm not reviewing both PR's and I can't tell what you've changed and what the other PR is chaning.
If you base a PR on top of another existing branch I think we need to review against that other PR until it's merged so that the review is only of your current changes not everything from both branches.
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.
Just watching the asciinema and again I don't know for sure if this is feedback from your PR or the branch you're building on top of but I'll note what I see either way.
- You ran a command 'ag' which I'm guessing is an alias or shortcut you've setup yourself? If so you shouldn't do that in a demo I don't know what that command does, hence I can't understand what you're trying to show us by running it.
- You've exposed your REPLICATED_API_TOKEN please rotate it and be more careful in the future about recording sensitive values.
- The output here is far to verbose, that's part of the cause of item 2 above, slim it down we shouldn't be showing task commands like you show here. Especially when they have API TOKENS in them. That goes for both the green and the white text, it's noisy look at the Embedded Cluster CLI outputs for a target of what good looks like. I think just disable all of the verbose output would be a good start.
- As I mentioned in yesterdays' standup, you shouldn't need cluster-create and setup-kubeconfig in your dependencies. As far as I can tell just setup-kubeconfig is sufficient because it already includes creating a cluster. We need to be careful not to unnecessarily re-run and steps for no reason.
- This is the biggest one, you're using a bash loop to make this work which is fine for development purposes however how do you see this working in a real install? An end user won't be using Taskfile or this repository, the preflight needs to work through Enterprise Portal and Embedded Cluster installs. Are you addressing that next? I'm fine with this implementation for developers but before we land anything we need to know what this means for end users.
many thanks @chris-sanders
|
I've shared the Loom in our Slack channel, tldr;
|
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 just realized this is still targeting main and not the other PR.
I'm assuming all changes in this PR are yours since you asked for another review, is that accurate?
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
name: cert-manager-preflights | ||
labels: | ||
troubleshoot.sh/kind: preflight | ||
type: Opaque | ||
stringData: | ||
preflight.yaml: | | ||
apiVersion: troubleshoot.sh/v1beta2 | ||
kind: Preflight | ||
metadata: | ||
name: cert-manager-preflights | ||
spec: | ||
analyzers: | ||
- distribution: | ||
outcomes: | ||
- pass: | ||
when: "== gke" | ||
message: GKE is a supported platform | ||
- pass: | ||
when: "== k3s" | ||
message: K3S is a supported platform | ||
- fail: | ||
when: "== kind" | ||
message: This application does not support Kind | ||
- warn: | ||
message: The Kubernetes platform is not validated, but there are no known compatibility issues. |
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.
Let's not use any 'dummy' values. Cert-manager has a list of prerequisites documented. Use real requirements.
Hi @chris-sanders, I can't target the other PR branch as the branch is in a forked repo. As mentioned earlier, I will wait for that PR to be merged to main and rebase this PR. I've updated the preflights for cert-manager accordingly. I can only find the k8s version as the only prerequisite here |
I didn't realize the other PR was in a forked repo, I'll mention to the team to work out of this repo in the future. |
5759745
to
05470e0
Compare
e7d1870
to
34e4f2d
Compare
Add Preflight Checks to WireGuard Easy (wg-easy)
Overview
This PR adds preflight checks to the
wg-easy
chart.Implementation Details
Each chart-specific preflight is defined as a Kubernetes Secret with label
troubleshoot.sh/kind: preflight
Preflight specs are stored in each chart's templates directory:
Added new task
helm-preflight
to streamline verificationUpdated container image to include the
preflight
binaryCurrent Status
cert-manager
preflight is currently a placeholder to demonstrate multiple preflight specs merging, not the final preflight contentwg-easy
preflight has been implemented with actual checks relevant to the chartTesting
Locally verify preflight specs with:
Next Steps