st-tech / gatling-operator

Automating distributed Gatling load testing using Kubernetes operator
MIT License
70 stars 19 forks source link

Fix manifests-release to exclude version from all-in-one manifest name #36

Closed ksudate closed 2 years ago

ksudate commented 2 years ago

Description

Remove the version information from the yaml generated by manifests-release. The reason for removing it is that there is no need to put the version in the manifest since each version is uploaded separately.

before change: https://github.com/st-tech/gatling-operator/releases/download/v0.4.0/gatling-operator-0.4.0.yaml after change: https://github.com/st-tech/gatling-operator/releases/download/v0.4.0/gatling-operator.yaml

Test

I've setup dummy trigger in CI and I've tagged with dummy one v0.4.0-dummy and got an expected result like this:

release url: https://github.com/st-tech/gatling-operator/releases/tag/v0.4.0-dummy action url: https://github.com/st-tech/gatling-operator/actions/runs/1750651152

スクリーンショット 2022-01-26 21 13 16

TODO: After the PR has been merged, remove both the dummy release and the tags

yokawasa commented 2 years ago

@tmrekk121 @kayamin

that there is no need to put the version in the manifest since each version is uploaded separately.

I agree upon this.

From permalink perspective, no version name needs to be included in gatling all-in-one manifest file name as it already has unique permalink for each version.

https://github.com/st-tech/gatling-operator/releases/download/<VERSION>/gatling-operator.yaml

VERSION added in manifest file name not necessarily needed. It's rather redundant

https://github.com/st-tech/gatling-operator/releases/download/<VERSION>/gatling-operator-<VERSION>.yaml

From UX perspective, it may be fetched and applied either directly or indirectly like this, and I don't see a strong reason for the version to be included in the all-in-one manifest file name.

# direct pattern 
kubectl apply -f https://github.com/st-tech/gatling-operator/releases/download/<VERSION>/gatling-operator.yaml

# indirect pattern
curl -sL https://github.com/st-tech/gatling-operator/releases/download/<VERSION>/gatling-operator.yaml -o gatling-operator.yaml
kubectl apply -f gatling-operator.yaml

# Or to avoid overwrite on local directory, it can be fetched like this
curl -sL https://github.com/st-tech/gatling-operator/releases/download/<VERSION>/gatling-operator.yaml -o gatling-operator-<VERSION>.yaml

I prefer to a simpler way

ksudate commented 2 years ago

@kayamin thanks for your review! When we install the gatling-operator, you will type a command like the following.

kubectl apply -f https://github.com/st-tech/gatling-operator/releases/download/v0.4.0/gatling-operator-0.4.0.yaml

You will see that the version information is not needed with this command.

For example, cert-manager uses this method.

kubectl apply -f https://github.com/jetstack/cert-manager/releases/download/v1.6.1/cert-manager.yaml

https://cert-manager.io/docs/installation/

kayamin commented 2 years ago

@tmrekk121 @yokawasa I got it! Sorry I didn't assume applying manifest by fetching manifest from remote url.

In that case, I agree with this PR purpose and contents 👍

ksudate commented 2 years ago

@yokawasa @kayamin Thanks for the review. I'll go ahead to merge this

yokawasa commented 2 years ago

@tmrekk121 thanks for the contribution! Just for your information, I've renamed all all-in-one manifest file names in past releases to gatling-operator.yaml from gatling-operator-<VERSION>.yaml

ksudate commented 2 years ago

thank you for the support!