Skip to content

Conversation

vish-k
Copy link
Contributor

@vish-k vish-k commented Jan 25, 2023

…nfig

Pull Request check list

  • [ x] Commit conforms to CONTRIBUTING.md?
  • [ x] Proper tests/regressions included?
  • [ x] Documentation updated?

Affected functionality

Description of change

Which issue this PR fixes
fixes #3497 Remove deprecated "enabled" flag from InMem telemetry config

azdagron
azdagron previously approved these changes Jan 26, 2023
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Thanks, @vish-k !

@azdagron
Copy link
Member

Looks like we need DCO signoff on your commit.l You can follow the details of the failed DCO workflow for a description of how to solve.

@vish-k
Copy link
Contributor Author

vish-k commented Jan 26, 2023

Thanks for your patience. I followed the steps to add DCO commit. Pls let me know if this works.

@azdagron
Copy link
Member

Looks like it is still missing. I'd rebase the commit on origin/main and then do a git commit -s --amend to add the signoff, and then force push to the PR branch.

@azdagron azdagron added this to the 1.6.0 milestone Jan 26, 2023
@azdagron
Copy link
Member

DCO signoff is still unhappy.

@vish-k
Copy link
Contributor Author

vish-k commented Jan 27, 2023

It's still complaining about earlier commit [17d7827] which I deleted. The latest commit has "Signed-off-by". Is there any way to bypass check for deleted commit? I'm sorry, this is my first commit so I'm messing this up. Should I just redo the fork and create new PR?

@azdagron
Copy link
Member

That commit is still part of the PR branch. I suggest rebasing your PR branch on top of main instead of merging main.

@vish-k
Copy link
Contributor Author

vish-k commented Jan 27, 2023

Since I was working a fork in my repo, I didn't create separate branch. I just made changes to local main branch, pushed to main in my repo/main and PR is merge my main into spire/main.

@azdagron
Copy link
Member

Sure, that's fine. So the main branch on your fork is the PR branch. I'd suggest rebasing main on your fork on top of main from this repo.

@azdagron
Copy link
Member

Unfortunately DCO is not something we can skip. If you hit me up on the SPIFFE slack, I'd be happy to help you get this sorted.

@azdagron
Copy link
Member

Looks like the linter is failing:

pkg/common/telemetry/inmem_test.go:13: unnecessary leading newline (whitespace)

@vish-k
Copy link
Contributor Author

vish-k commented Jan 27, 2023 via email

@vish-k
Copy link
Contributor Author

vish-k commented Jan 28, 2023

It's failing with "unnecessary leading newline (whitespace)" but there is no whitespace or newline.

I tried vs code, gofmt and even nolint directive for golangci-lint but no luck so far. Do you have any suggestion?

@@ -11,8 +11,6 @@ import (
)

func TestInMem(t *testing.T) {
enabled := true
disabled := false

Copy link
Member

Choose a reason for hiding this comment

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

Remove this empty line at the beginning of the TestInMem function to satisfy the linter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! fixed it.

@azdagron azdagron merged commit 1681c0b into spiffe:main Jan 29, 2023
stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023
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.

2 participants