kubesaw / ksctl

ksctl is a command-line tool to manage your installation of KubeSaw
Apache License 2.0
4 stars 11 forks source link

do not install user-identity-mapper #62

Closed MatousJobanek closed 3 months ago

MatousJobanek commented 3 months ago

KUBESAW-169

there is no need to install the user-identity-mapper binary locally (together with the ksctl binary), so the PR is moving the user-identity-mapper directory out of cmd folder

MatousJobanek commented 3 months ago

having both ksctl and user-identity-mapper under cmd make it clear that we have 2 CLIs

user-identity-mapper is not really a cli that you would run locally on the daily basis. it's more to be used as a container.

Anyway, how would you change the make install command so it installs only ksctl binary? When you run make install now, then it installs ksctl as well as user-identity-mapper, and there is no need to install user-identity-mapper locally.

xcoulon commented 3 months ago

having both ksctl and user-identity-mapper under cmd make it clear that we have 2 CLIs

user-identity-mapper is not really a cli that you would run locally on the daily basis. it's more to be used as a container.

well, it's a CLI nonetheless, so it makes sense to keep it under the cmd folder IMO

Anyway, how would you change the make install command so it installs only ksctl binary? When you run make install now, then it installs ksctl as well as user-identity-mapper, and there is no need to install user-identity-mapper locally.

Sure, then it's just a matter of updating the install makefile goal so we only have the ksctl binary when running make install, and have a separate makefile goal used when building the Docker image?

MatousJobanek commented 3 months ago

We don't need to build the user-identity-mapper via a makefile target at all - this is done in the Docker image when it's being built: https://github.com/kubesaw/ksctl/blob/3b26733db38131551bc1fe08ff9dd6853ca50e62/cmd/user-identity-mapper/Dockerfile#L16-L19

so again, how would you change the go install command so it installs ksctl only (using the go native functionality, which means also in the location of either GOBIN or GOPATH)? I tried that, but I'm doing some mistake there as I cannot achieve the expected outcome, or I'm missing some important flag. Any help is more than welcome :-)

xcoulon commented 3 months ago

We don't need to build the user-identity-mapper via a makefile target at all - this is done in the Docker image when it's being built:

https://github.com/kubesaw/ksctl/blob/3b26733db38131551bc1fe08ff9dd6853ca50e62/cmd/user-identity-mapper/Dockerfile#L16-L19

right, so we already have all what we need for this CLI

so again, how would you change the go install command so it installs ksctl only (using the go native functionality, which means also in the location of either GOBIN or GOPATH)? I tried that, but I'm doing some mistake there as I cannot achieve the expected outcome, or I'm missing some important flag. Any help is more than welcome :-)

$(Q)CGO_ENABLED=0 \
    go ${GO_COMMAND} ${V_FLAG} \
    -ldflags "-X ${GO_PACKAGE_PATH}/pkg/version.Commit=${GIT_COMMIT_ID} -X       
         ${GO_PACKAGE_PATH}/pkg/version.BuildTime=${BUILD_TIME}" \
        ${GO_EXTRA_FLAGS} ${GO_PACKAGE_PATH}/cmd/ksctl/...

with ${GO_PACKAGE_PATH}/cmd/ksctl/... instead of ${GO_PACKAGE_PATH}/cmd/... seems to work on my machine: I did not have a user-identity-mapper binary after running make install

mfrancisc commented 3 months ago

with ${GO_PACKAGE_PATH}/cmd/ksctl/... instead of ${GO_PACKAGE_PATH}/cmd/... seems to work on my machine: I did not have a user-identity-mapper binary after running make install

works for me too! I was about to suggest the same thing.

MatousJobanek commented 3 months ago

ok, there was some error between my chair and my keyboard. I was almost 100% sure that I tried that and it didn't built the ksctl binary properly. I maybe omitted the last three dots /... :man_shrugging: :man_facepalming: Anyway, thanks for your proposal Closing this PR...

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 68.43%. Comparing base (3b26733) to head (dbd4849).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #62 +/- ## ======================================= Coverage 68.43% 68.43% ======================================= Files 43 43 Lines 2474 2474 ======================================= Hits 1693 1693 Misses 591 591 Partials 190 190 ``` | [Files](https://app.codecov.io/gh/kubesaw/ksctl/pull/62?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesaw) | Coverage Δ | | |---|---|---| | [user-identity-mapper/cmd/user\_identity\_mapper.go](https://app.codecov.io/gh/kubesaw/ksctl/pull/62?src=pr&el=tree&filepath=user-identity-mapper%2Fcmd%2Fuser_identity_mapper.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesaw#diff-dXNlci1pZGVudGl0eS1tYXBwZXIvY21kL3VzZXJfaWRlbnRpdHlfbWFwcGVyLmdv) | `100.00% <ø> (ø)` | |