fix: dont emit empty strings when reading role #66

TJM · 2023-04-18T03:08:13Z

Now when the role is read, it looks like:

$ vault read artifactory/roles/test
Key            Value
---            -----
default_ttl    2h
max_ttl        3h
role           test
scope          applied-permissions/groups:readers

alexhung

Out of interest, have you tried running acceptance tests using make acceptance? Want to know if it works for others outside JFrog dev team.

alexhung · 2023-04-18T16:08:40Z

README.md

@@ -283,7 +298,7 @@ make artifactory

 ```sh
 export VAULT_ADDR=http://localhost:8200
-export VAULT_token=root
+export VAULT_TOKEN=root


TJM · 2023-04-18T17:04:48Z

Out of interest, have you tried running acceptance tests using make acceptance? Want to know if it works for others outside JFrog dev team.

I just tried it, and it didn't go well.. I had "make start" in one window, "make artifactory" and "make setup" were done, but "make acceptance" failed a lot :)

[tmcneely@local artifactory-secrets-plugin]$ make acceptance
export VAULT_ACC=true && \
		go test -run TestAcceptance -cover -coverprofile=coverage.txt -v -p 1 -timeout 5m ./...
Logging in to Artifactory (http://localhost:8082) as admin ...
Generating artifactory admin access token.
=== RUN   TestAcceptanceBackend_PathRotate
2023-04-18T11:06:27.005-0600 [WARN]  error making system version request: response=<nil> err="parse "": empty url"
    test_utils.go:44: parse "": empty url
--- FAIL: TestAcceptanceBackend_PathRotate (0.00s)
=== RUN   TestAcceptanceBackend_PathConfig
=== RUN   TestAcceptanceBackend_PathConfig/update
    test_utils.go:91:
        	Error Trace:	test_utils.go:91
        	Error:      	Expected nil, but got: &logical.Response{Secret:<nil>, Auth:<nil>, Data:map[string]interface {}{"error":"access_token is required"}, Redirect:"", Warnings:[]string(nil), WrapInfo:(*wrapping.ResponseWrapInfo)(nil), Headers:map[string][]string(nil)}
        	Test:       	TestAcceptanceBackend_PathConfig/update
=== RUN   TestAcceptanceBackend_PathConfig/read
    test_utils.go:103:
        	Error Trace:	test_utils.go:103
        	Error:      	Should NOT be empty, but was <nil>
        	Test:       	TestAcceptanceBackend_PathConfig/read
=== RUN   TestAcceptanceBackend_PathConfig/delete
--- FAIL: TestAcceptanceBackend_PathConfig (0.00s)
    --- FAIL: TestAcceptanceBackend_PathConfig/update (0.00s)
    --- FAIL: TestAcceptanceBackend_PathConfig/read (0.00s)
    --- PASS: TestAcceptanceBackend_PathConfig/delete (0.00s)
=== RUN   TestAcceptanceBackend_PathRole
=== RUN   TestAcceptanceBackend_PathRole/configure_backend
    test_utils.go:91:
        	Error Trace:	test_utils.go:91
        	Error:      	Expected nil, but got: &logical.Response{Secret:<nil>, Auth:<nil>, Data:map[string]interface {}{"error":"access_token is required"}, Redirect:"", Warnings:[]string(nil), WrapInfo:(*wrapping.ResponseWrapInfo)(nil), Headers:map[string][]string(nil)}
        	Test:       	TestAcceptanceBackend_PathRole/configure_backend
=== RUN   TestAcceptanceBackend_PathRole/create
    test_utils.go:202:
        	Error Trace:	test_utils.go:202
        	Error:      	Expected nil, but got: &logical.Response{Secret:<nil>, Auth:<nil>, Data:map[string]interface {}{"error":"backend not configured"}, Redirect:"", Warnings:[]string{"Endpoint replaced the value of these parameters with the values captured from the endpoint's path: [role]"}, WrapInfo:(*wrapping.ResponseWrapInfo)(nil), Headers:map[string][]string(nil)}
        	Test:       	TestAcceptanceBackend_PathRole/create
=== RUN   TestAcceptanceBackend_PathRole/read
    test_utils.go:212:
        	Error Trace:	test_utils.go:212
        	Error:      	Expected value not to be nil.
        	Test:       	TestAcceptanceBackend_PathRole/read
--- FAIL: TestAcceptanceBackend_PathRole (0.00s)
    --- FAIL: TestAcceptanceBackend_PathRole/configure_backend (0.00s)
    --- FAIL: TestAcceptanceBackend_PathRole/create (0.00s)
    --- FAIL: TestAcceptanceBackend_PathRole/read (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x1641e51]

goroutine 26 [running]:
testing.tRunner.func1.2({0x16e2b60, 0x1d06900})
	/Users/tmcneely/.asdf/installs/golang/1.20.3/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/Users/tmcneely/.asdf/installs/golang/1.20.3/go/src/testing/testing.go:1529 +0x39f
panic({0x16e2b60, 0x1d06900})
	/Users/tmcneely/.asdf/installs/golang/1.20.3/go/src/runtime/panic.go:884 +0x213
github.com/jfrog/artifactory-secrets-plugin.(*accTestEnv).ReadPathRole(0xc0002a6d20, 0x10cdc17?)
	/Users/tmcneely/Projects/Artifactory/artifactory-secrets-plugin/test_utils.go:215 +0x131
testing.tRunner(0xc00026fd40, 0xc000297170)
	/Users/tmcneely/.asdf/installs/golang/1.20.3/go/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/Users/tmcneely/.asdf/installs/golang/1.20.3/go/src/testing/testing.go:1629 +0x3ea
FAIL	github.com/jfrog/artifactory-secrets-plugin	0.486s
?   	github.com/jfrog/artifactory-secrets-plugin/cmd/artifactory	[no test files]
FAIL
make: *** [acceptance] Error 1

NOTE: Vault and Artifactory are still running, it was the test that segfaulted.
(second paste, I added the exports for ARTIFACTORY_URL and JFROG_ACCESS_TOKEN

EDIT: This has been fixed, see below. The variables are actually JFROG_URL and ARTIFACTORY_ACCESS_TOKEN (we will change that in a future MR)

alexhung · 2023-04-18T17:49:33Z

Makefile

-ARTIFACTORY_URL ?= http://localhost:8082
-JFROG_ACCESS_TOKEN ?= $(shell TOKEN_USERNAME=$(TOKEN_USERNAME) ARTIFACTORY_URL=$(ARTIFACTORY_URL) ./scripts/getArtifactoryAdminToken.sh)
+export ARTIFACTORY_URL ?= http://localhost:8082
+export JFROG_ACCESS_TOKEN ?= $(shell TOKEN_USERNAME=$(TOKEN_USERNAME) ARTIFACTORY_URL=$(ARTIFACTORY_URL) ./scripts/getArtifactoryAdminToken.sh)


👍🏼 I export them in my shell manually. Hence I didn't run into this problem until it's being run in clean env.

I even tried with them manually exported and got very similar output

The tests actually are using JFROG_URL and ARTIFACTORY_ACCESS_TOKEN. I guess we should update them to use the same...

Ah, the readme says ARTIFACTORY_URL and JFROG_ACCESS_TOKEN :) .. yes, do you want me to add that to this PR or is it already bloated enough? :)

Let's have separate PR 😄

[tmcneely@local artifactory-secrets-plugin]$ make acceptance export VAULT_ACC=true && \ go test -run TestAcceptance -cover -coverprofile=coverage.txt -v -p 1 -timeout 5m ./... Logging in to Artifactory (http://localhost:8082) as admin ... Generating artifactory admin access token. === RUN TestAcceptanceBackend_PathRotate 2023-04-18T12:26:49.916-0600 [DEBUG] oldToken ID: ddc8d463-0fd1-42d0-b2fe-6a25b514566e 2023-04-18T12:26:49.924-0600 [DEBUG] newTokenID: 2e576145-e34e-41a7-aa48-5d812bfcd28f --- PASS: TestAcceptanceBackend_PathRotate (2.14s) === RUN TestAcceptanceBackend_PathConfig === RUN TestAcceptanceBackend_PathConfig/update === RUN TestAcceptanceBackend_PathConfig/read === RUN TestAcceptanceBackend_PathConfig/delete --- PASS: TestAcceptanceBackend_PathConfig (0.01s) --- PASS: TestAcceptanceBackend_PathConfig/update (0.00s) --- PASS: TestAcceptanceBackend_PathConfig/read (0.01s) --- PASS: TestAcceptanceBackend_PathConfig/delete (0.00s) === RUN TestAcceptanceBackend_PathRole === RUN TestAcceptanceBackend_PathRole/configure_backend === RUN TestAcceptanceBackend_PathRole/create === RUN TestAcceptanceBackend_PathRole/read === RUN TestAcceptanceBackend_PathRole/delete === RUN TestAcceptanceBackend_PathRole/cleanup_backend --- PASS: TestAcceptanceBackend_PathRole (0.00s) --- PASS: TestAcceptanceBackend_PathRole/configure_backend (0.00s) --- PASS: TestAcceptanceBackend_PathRole/create (0.00s) --- PASS: TestAcceptanceBackend_PathRole/read (0.00s) --- PASS: TestAcceptanceBackend_PathRole/delete (0.00s) --- PASS: TestAcceptanceBackend_PathRole/cleanup_backend (0.00s) === RUN TestAcceptanceBackend_PathTokenCreate === RUN TestAcceptanceBackend_PathTokenCreate/configure_backend === RUN TestAcceptanceBackend_PathTokenCreate/create_role === RUN TestAcceptanceBackend_PathTokenCreate/create_token_for_role === RUN TestAcceptanceBackend_PathTokenCreate/delete_role === RUN TestAcceptanceBackend_PathTokenCreate/cleanup_backend --- PASS: TestAcceptanceBackend_PathTokenCreate (0.02s) --- PASS: TestAcceptanceBackend_PathTokenCreate/configure_backend (0.01s) --- PASS: TestAcceptanceBackend_PathTokenCreate/create_role (0.00s) --- PASS: TestAcceptanceBackend_PathTokenCreate/create_token_for_role (0.01s) --- PASS: TestAcceptanceBackend_PathTokenCreate/delete_role (0.00s) --- PASS: TestAcceptanceBackend_PathTokenCreate/cleanup_backend (0.00s) PASS github.com/jfrog/artifactory-secrets-plugin coverage: 62.7% of statements ok github.com/jfrog/artifactory-secrets-plugin 2.648s coverage: 62.7% of statements ? github.com/jfrog/artifactory-secrets-plugin/cmd/artifactory [no test files]

TJM requested a review from alexhung as a code owner April 18, 2023 03:08

TJM added 2 commits April 17, 2023 21:29

fix: README - add prerequisites

d13d2c9

fix: makefile updates after build changes

e7289eb

TJM force-pushed the fix/empty-strings branch from d378a86 to 7c8360a Compare April 18, 2023 03:29

feat: don't output empty fields in role

edaa864

TJM force-pushed the fix/empty-strings branch from 7c8360a to edaa864 Compare April 18, 2023 03:31

alexhung previously approved these changes Apr 18, 2023

View reviewed changes

alexhung requested a review from shrajfr12 April 18, 2023 16:10

fix: add exports for acceptance test

5b53dda

TJM dismissed alexhung’s stale review via 5b53dda April 18, 2023 17:08

alexhung reviewed Apr 18, 2023

View reviewed changes

alexhung approved these changes Apr 18, 2023

View reviewed changes

TJM mentioned this pull request Apr 18, 2023

make acceptance uses inconsistent env variables #70

Closed

alexhung mentioned this pull request Apr 18, 2023

Fix/acceptance-vars #71

Merged

alexhung merged commit d8a29c0 into jfrog:master Apr 18, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: dont emit empty strings when reading role #66

fix: dont emit empty strings when reading role #66

TJM commented Apr 18, 2023

alexhung left a comment

alexhung Apr 18, 2023

TJM commented Apr 18, 2023 •

edited

alexhung Apr 18, 2023

TJM Apr 18, 2023

alexhung Apr 18, 2023

TJM Apr 18, 2023

alexhung Apr 18, 2023

TJM Apr 18, 2023

fix: dont emit empty strings when reading role #66

fix: dont emit empty strings when reading role #66

Conversation

TJM commented Apr 18, 2023

alexhung left a comment

Choose a reason for hiding this comment

alexhung Apr 18, 2023

Choose a reason for hiding this comment

TJM commented Apr 18, 2023 • edited

alexhung Apr 18, 2023

Choose a reason for hiding this comment

TJM Apr 18, 2023

Choose a reason for hiding this comment

alexhung Apr 18, 2023

Choose a reason for hiding this comment

TJM Apr 18, 2023

Choose a reason for hiding this comment

alexhung Apr 18, 2023

Choose a reason for hiding this comment

TJM Apr 18, 2023

Choose a reason for hiding this comment

TJM commented Apr 18, 2023 •

edited