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

allow default-token and per-subscription tokens in client.yml #659

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

wunter8
Copy link
Contributor

@wunter8 wunter8 commented Mar 6, 2023

fixes #653

@@ -130,7 +133,7 @@ func TestCLI_Publish_Wait_PID_And_Cmd(t *testing.T) {
require.Equal(t, `command failed: does-not-exist-no-really "really though", error: exec: "does-not-exist-no-really": executable file not found in $PATH`, err.Error())

// Tests with NTFY_TOPIC set ////
require.Nil(t, os.Setenv("NTFY_TOPIC", topic))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests after this one were failing because they kept connecting to the wrong ntfy server. It took me hours to figure out that this was the reason; the environment variable was persisting after this test and was affecting the remaining tests in the file.

Copy link
Owner

Choose a reason for hiding this comment

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

I didn't know you can do that t.Setenv. Wow. Sorry about that.

@wunter8
Copy link
Contributor Author

wunter8 commented Mar 7, 2023

I realize it's a bit sloppy to include the latest commit here instead of in a second PR, but I needed it to pass the tests and eliminate the race condition in the old version of the tests here.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 75.60% and project coverage change: +0.38 🎉

Comparison is base (477c9d3) 67.15% compared to head (758eb3f) 67.53%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #659      +/-   ##
==========================================
+ Coverage   67.15%   67.53%   +0.38%     
==========================================
  Files          47       47              
  Lines        6631     6642      +11     
==========================================
+ Hits         4453     4486      +33     
+ Misses       1484     1465      -19     
+ Partials      694      691       -3     
Impacted Files Coverage Δ
cmd/publish.go 74.23% <63.15%> (+4.23%) ⬆️
cmd/subscribe.go 44.72% <84.21%> (+14.85%) ⬆️
client/config.go 75.00% <100.00%> (+1.66%) ⬆️
server/server.go 62.71% <100.00%> (+0.17%) ⬆️
user/manager.go 59.29% <0.00%> (-0.71%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -130,7 +133,7 @@ func TestCLI_Publish_Wait_PID_And_Cmd(t *testing.T) {
require.Equal(t, `command failed: does-not-exist-no-really "really though", error: exec: "does-not-exist-no-really": executable file not found in $PATH`, err.Error())

// Tests with NTFY_TOPIC set ////
require.Nil(t, os.Setenv("NTFY_TOPIC", topic))
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't know you can do that t.Setenv. Wow. Sorry about that.

@binwiederhier binwiederhier merged commit 3f754f2 into binwiederhier:main Mar 7, 2023
@binwiederhier
Copy link
Owner

I just merged this 600 line PR without a single comment other than "wow i didn't know that". Those that know me know that that is pretty much an impossible thing to achieve. Great work and 100 thanks.

@wunter8
Copy link
Contributor Author

wunter8 commented Mar 7, 2023

I didn't even realize this was 600 lines, haha. I knew it was big, but I didn't expect it was that big

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add default-token option to client.yml
3 participants