Skip to content
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

More makefile fixes #79

Merged
merged 2 commits into from
Apr 25, 2023
Merged

More makefile fixes #79

merged 2 commits into from
Apr 25, 2023

Conversation

TJM
Copy link
Contributor

@TJM TJM commented Apr 22, 2023

I noticed that make upgrade was not working. I also noticed that the plugin name was "artifactory-secrets-plugin" instead of "artifactory" so I tried to work on that.

  • Makefile variables that are static should use :=, especially ones that use shell commands to evaluate
  • consistent interpolation, I think Makefile recommends using $(VAR_NAME) (rather than curly braces which can sometimes be mistaken for shell variables)
  • make register - only register
  • make upgrade (add build that was part of register)
  • update PHONY
  • Set PLUGIN_NAME separately from PLUGIN_FILE (to prevent future filename changes from affecting as much)
  • make plugin name artifactory (instead of artifactory-secrets-engine)
  • Make the "dev/test" procedure more closely match the intended use case (README)
  • make setup needs to register (if plugin name is artifactory)

@TJM TJM force-pushed the fix/more-makefile branch 3 times, most recently from e8cd0e6 to 5c361df Compare April 22, 2023 03:28
@TJM
Copy link
Contributor Author

TJM commented Apr 22, 2023

I was thinking of adding something like:

GIT_DIRTY := $(shell test -n "`git status --porcelain`" && echo "+CHANGES" || true)

... and add it to the version string, but the git commit hash is already behind a +, so it causes an error, due to it not being a semantic version. We could do "-CHANGES" but... (see thread on make upgrade)

~tommy

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated

enable:
vault secrets enable -path=${PLUGIN_PATH} -plugin-version=${NEXT_VERSION} ${PLUGIN_FILE}
vault secrets enable -path=$(PLUGIN_PATH) -plugin-version=$(NEXT_VERSION) $(PLUGIN_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

This should remain PLUGIN_PATH.

The Vault help doc clearly use 'path' for vault secrets enable/disable command.

Usage: vault secrets disable [options] PATH

Whereas it uses 'name' (synonymous with file name here) for vault plugin ... commands.

Usage: vault plugin register [options] TYPE NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"enable" should use the registered plugin name

Copy link
Member

Choose a reason for hiding this comment

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

Just so happened that both PLUGIN_PATH and PLUGIN_NAME have same value ('artifactory'). I'm ok with using PLUGIN_NAME. May be rename PLUGIN_PATH to PLUGIN_FILE_PATH to reduce confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or maybe PLUGIN_VAULT_PATH ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have a better idea for this, I am open to it. I did have a case where I was trying to test on a "real" vault server (not the dev one) and I wanted to use something like test-artifactory... so I had already started this change for $(PLUGIN_PATH), but I agree that it is confusing, so PLUGIN_VAULT_PATH or VAULT_PLUGIN_PATH or ????

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@alexhung
Copy link
Member

I'll review/approve this PR once you are ready and toggle this from 'Draft' 😄

Makefile Outdated Show resolved Hide resolved
@TJM TJM marked this pull request as ready for review April 24, 2023 19:12
@TJM
Copy link
Contributor Author

TJM commented Apr 24, 2023

Rebased to cut the number of "fix" commits down, this should be ready to go.

@TJM TJM mentioned this pull request Apr 24, 2023
4 tasks
@alexhung alexhung added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Apr 25, 2023
@alexhung alexhung merged commit ed80911 into jfrog:master Apr 25, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants