-
Notifications
You must be signed in to change notification settings - Fork 5
feat(wg-easy): add list cluster and Verify kubeconfig for taskfile #42
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
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.
A few questions. I like seeing the code and how you've implemented things. It would be helpful to understand why you're making some of these changes. We want to get the actual code landed in the repo, but we also want to have a team discussion about why we do different things.
A few of these look like we're just directly wrapping replicated
functions with Taskfile. That might be fine, but I'm curious to know why. I don't personally see the value in just wrapping commands that replicated cli provides but you might see how they will tie into a workflow in the future where you want them as dependencies or something as well.
DEVELOPMENT.md
Outdated
@@ -0,0 +1,93 @@ | |||
## 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.
I feel like this file has good information in it but doesn't belong in platform-examples directly. I suspect you checked this in by accident?
These look like they are all valid bugs if I'm reading this correctly?
Can you remove this file from the branch, turn these into shortcuts, and add them to the product feedback meeting to highlight as rough edges on the replicated CLI that we can highlight to ... the vendor portal team I think.
applications/wg-easy/.env
Outdated
VERSION=0.0.4 | ||
CHANNEL=wg-easy | ||
APP_NAME=wg-easy | ||
RELEASE_NOTES="Release created via task release-create" |
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.
This also feels like a file that you shouldn't have checked in. Aren't all of these values set by defaults already?
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.
Those values has been set as default as var
EMBEDDED: '{{.EMBEDDED | default "false"}}'
LICENSE_ID: '{{if eq .EMBEDDED "true"}}{{.LICENSE_ID | default "2cmqT1dBVHZ3aSH21kPxWtgoYGr"}}{{end}}'
TIMEOUT: '{{if eq .EMBEDDED "true"}}420{{else}}300{{end}}'
It is not flexible when I copy this taskfile and apply to my own app. I am trying to migrate those values back to .env.
I will remove .env and put it into some session of README.md
applications/wg-easy/Taskfile.yaml
Outdated
@@ -177,6 +206,17 @@ tasks: | |||
fi | |||
- echo "All matching clusters deleted and kubeconfig cleaned up!" | |||
|
|||
update-version: |
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'd like some conversation / explanation of what the intorduction of .env is how/why you see updating it like this etc. This might be a good pattern it's just not immediately obvious to me so I could use some explanation.
I tend to treat 'versions' as git tags. That's to say that the version is tracked with the source control and not files within it. In particular the concern I have with having the version in the repo is it seems to regularly end up in a situation where you need to release a new version like "RC1" moving to "Stable" where you don't want code changes, but now you have to change code todo that.
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.
This one, I am trying to aline with our makefile create-release cmd. You are right, let me know rethink it.
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.
the problem in a repo like platform-examples is that the git tag is for the entire repo, and here we want to version each of the example applications. I'd love to know a way we can say that wg-easy is version 1.2.3 but some other app is 0.4.1 and so on.
applications/wg-easy/Taskfile.yaml
Outdated
CHANNEL: '{{.CHANNEL | default "Unstable"}}' | ||
RELEASE_NOTES: '{{.RELEASE_NOTES | default "Release created via task release-create"}}' | ||
dotenv: ['.env'] | ||
cmds: | ||
- echo "Creating and promoting release for {{.APP_NAME}} to channel {{.CHANNEL}}..." | ||
- echo "Creating and promoting release for $APP_NAME to channel $CHANNEL..." | ||
- | | ||
# Create and promote the release in one step | ||
echo "Creating release from files in ./release directory..." | ||
replicated release create --app {{.APP_NAME}} --yaml-dir ./release --release-notes "{{.RELEASE_NOTES}}" --promote {{.CHANNEL}} | ||
|
||
echo "Release created and promoted to channel {{.CHANNEL}}" | ||
replicated release create --app $APP_NAME --yaml-dir ./release --release-notes "$RELEASE_NOTES" --promote $CHANNEL --version $VERSION | ||
echo "Release version $VERSION created and promoted to channel $CHANNEL" |
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.
What's the benefit / advantage of moving this out of taskfile variables?
In this case I like not using the environment mainly because I'm concerned about a users environment being tainted. For example, someone forgot they exported an app name before lunch and then push this app over top of a different application b/c they didn't close their terminal.
The Taskfile variables are automatically set to the right value and can be adjusted at runtime which to me fit well. However, I'm curious if you have thoughts on why you're changing this.
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.
Basically, moving variables back to .env. The intension is use .env to pass all the vars from a static file which can be tracked. If they are keep using the same .env file without any change, everything should constant
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 comment as above, I'm not a fan of moving things to the ENV or moving things to a file in the repo.
I personally recommend checking out https://direnv.net/ that's what I use, but I don't check it in. This helps me set and remove my environment based on my file path. But I don't check it into the repo.
I'm undecided if Taskfile variables or ENV is better but I tend to lean toward Taskfile as a first pass purely from a "try using the tool with it's provided features before starting to develop your own path". Maybe the Taskfile design is this way intentionally and we don't need to reinvent it.
applications/wg-easy/Taskfile.yaml
Outdated
deps: | ||
- prepare-release | ||
|
||
release-inspect: |
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.
The docs on our website doesn't really say why this is useful. What do you use this for? Is this used as part of the workflow or is it just wrapping the replicated cli?
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.
This is when I create a release and check it in my vendor port, I found a lot of errors from editor lint which may confuse users. That's one the reason to bring release inspect back. If it is not useful, I will remove it
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'm actually still not completely clear what you mean.
Do you mean that you had errors that you didn't know about until later and running this would have told you?
Do you mean errors show up in the UI and you didn't know about it until you went to Vendor Portal? Did they matter? I tend to think the linting is broken enough my personal recommendation right now is to ignore it. I think it causes more trouble than good.
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 will explain this part in a google doc
Thank @chris-sanders and @xavpaice's advise. I will move other complicated parts into another PR. |
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
sc-121915