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

golang: add test coverage, and fix handling of files with multiple build-tags #14

Merged
merged 5 commits into from
Oct 2, 2023

Conversation

@thaJeztah thaJeztah changed the title gaoling: add test coverage, and fix handling of files with multiple build-tags golang: add test coverage, and fix handling of files with multiple build-tags Aug 23, 2022
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah marked this pull request as ready for review August 24, 2022 12:39
@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 24, 2022

This is ready for review; @kunalkushwaha @AkihiroSuda @estesp PTAL

(easiest to step through each commit - I tried to outline/describe each change I made)

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

@kunalkushwaha PTAL 🙏

@thaJeztah
Copy link
Member Author

@kunalkushwaha @AkihiroSuda ptal 🤗

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see comments

golang_test.go Outdated Show resolved Hide resolved
}
if strings.HasPrefix(string(buf2), "// +build ") {
buildTags = append(buildTags, string(buf2))
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you want an else break case here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, yes, doesn't hurt I guess 😄

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This looks to be how it's used in current projects; also add a test-
case with a single build-tag.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

@mikebrow updated; PTAL

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

@kunalkushwaha PTAL - wondering if you would be open to transferring this repository to the containerd org and/or integrate it into https://github.com/containerd/project-checks

@kunalkushwaha
Copy link
Collaborator

Hi @thaJeztah , Sure.

Sorry for not being active with the PRs. Moved to other work with a change in my organization.

@kunalkushwaha
Copy link
Collaborator

LGTM

@kunalkushwaha
Copy link
Collaborator

Hi @thaJeztah , I am not able to transfer this project to containerd org.

Please help in transferring the project.

@kunalkushwaha kunalkushwaha merged commit b0cfa33 into containerd:master Oct 2, 2023
1 check passed
@thaJeztah thaJeztah deleted the fix_golang branch October 2, 2023 23:00
@thaJeztah
Copy link
Member Author

Ah, yes probably needs an admin for the containerd org; @dmcgowan are you able to help with that (if we're okay with moving the repo that is)

@dmcgowan
Copy link
Member

dmcgowan commented Oct 2, 2023

Yeah, I can help you get that moved. There are a few different options, you can give me admin on this repo or I can get you into the containerd org for the transfer. You can also try to transfer it to me, I don't think the transfer requests work anymore to orgs, only to other users.

@kunalkushwaha
Copy link
Collaborator

kunalkushwaha commented Oct 3, 2023

Thanks @thaJeztah. @dmcgowan : I made a request to transfer this repo to your user. Please take it forward.

@dmcgowan
Copy link
Member

dmcgowan commented Oct 3, 2023

Done! Thanks

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.

5 participants