Skip to content

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

Merged
merged 12 commits into from
Apr 17, 2025

Conversation

DexterYan
Copy link
Member

@DexterYan DexterYan commented Apr 7, 2025

sc-121915

  • add list cluster and verify-kubeconfig for taskfile
  • update applications/wg-easy/docs/task-reference.md

@DexterYan DexterYan marked this pull request as draft April 7, 2025 06:12
Copy link
Member

@chris-sanders chris-sanders left a 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
Copy link
Member

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.

Comment on lines 1 to 4
VERSION=0.0.4
CHANNEL=wg-easy
APP_NAME=wg-easy
RELEASE_NOTES="Release created via task release-create"
Copy link
Member

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?

Copy link
Member Author

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

@@ -177,6 +206,17 @@ tasks:
fi
- echo "All matching clusters deleted and kubeconfig cleaned up!"

update-version:
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Comment on lines 229 to 278
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"
Copy link
Member

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.

Copy link
Member Author

@DexterYan DexterYan Apr 9, 2025

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

Copy link
Member

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.

deps:
- prepare-release

release-inspect:
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

@DexterYan DexterYan changed the title WIP: implement for wg-easy feat(wg-easy): add list cluster and Verify kubeconfig for taskfile Apr 14, 2025
@DexterYan
Copy link
Member Author

DexterYan commented Apr 14, 2025

Thank @chris-sanders and @xavpaice's advise. I will move other complicated parts into another PR.
This pr will be a simple one.

@DexterYan DexterYan marked this pull request as ready for review April 14, 2025 09:12
Copy link
Member

@nvanthao nvanthao left a comment

Choose a reason for hiding this comment

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

LGTM

@DexterYan DexterYan merged commit fd85b13 into wg-easy Apr 17, 2025
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