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

ruby: support gemspec #836

Merged
merged 4 commits into from
Apr 4, 2023
Merged

ruby: support gemspec #836

merged 4 commits into from
Apr 4, 2023

Conversation

RTann
Copy link
Contributor

@RTann RTann commented Feb 17, 2023

No description provided.

@RTann RTann force-pushed the ruby-gem branch 4 times, most recently from 996ffe6 to 6f21125 Compare March 9, 2023 02:00
@RTann RTann marked this pull request as ready for review March 9, 2023 02:02
@RTann RTann requested a review from a team as a code owner March 9, 2023 02:02
@RTann RTann requested review from crozzy, daynewlee and hdonnay and removed request for a team March 9, 2023 02:02
@RTann
Copy link
Contributor Author

RTann commented Mar 9, 2023

I can make more tests in a bit, but I wanted to open this up for review anyway

@RTann RTann force-pushed the ruby-gem branch 3 times, most recently from afd694f to 5d52017 Compare March 17, 2023 18:10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand this is pretty large, but I'm not sure a better way to do this. I think we should have a Quay repo we can use for test images

Copy link
Contributor

Choose a reason for hiding this comment

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

We could try making use of the fetch.Layer() functionality as we do here: https://github.com/quay/claircore/blob/main/rpm/packagescanner_test.go#L1616. We already have https://quay.io/repository/projectquay/clair-fixtures I can skopeo cp as needed and/give you access to push (if you don't have it).

Comment on lines 154 to 182
func gems(ctx context.Context, sys fs.FS) (out []string, err error) {
return out, fs.WalkDir(sys, ".", func(p string, d fs.DirEntry, err error) error {
ev := zlog.Debug(ctx).
Str("file", p)
switch {
case err != nil:
ev.Discard().Send()
return err
case !d.Type().IsRegular():
ev.Discard().Send()
// Should we chase symlinks with the correct name?
return nil
case strings.HasPrefix(filepath.Base(p), ".wh."):
ev.Discard().Send()
return nil
case gemspecPath.MatchString(p):
ev = ev.Str("kind", "gem")
default:
ev.Discard().Send()
return nil
}
ev.Msg("found package")
out = append(out, p)
return nil
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do something like this to avoid the inevitable "add a case, forget to Discard"?

func gems(ctx context.Context, sys fs.FS) (out []string, err error) {
	return out, fs.WalkDir(sys, ".", func(p string, d fs.DirEntry, err error) error {
		ev := zlog.Debug(ctx).
			Str("file", p)
		defer func() {
			ev.Discard().Send()
		}()
		switch {
		case err != nil:
			return err
		case !d.Type().IsRegular():
			// Should we chase symlinks with the correct name?
			return nil
		case strings.HasPrefix(filepath.Base(p), ".wh."):
			return nil
		case gemspecPath.MatchString(p):
			ev = ev.Str("kind", "gem")
		default:
			return nil
		}
		ev.Msg("found package")
		out = append(out, p)
		return nil
	})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We could try making use of the fetch.Layer() functionality as we do here: https://github.com/quay/claircore/blob/main/rpm/packagescanner_test.go#L1616. We already have https://quay.io/repository/projectquay/clair-fixtures I can skopeo cp as needed and/give you access to push (if you don't have it).

@RTann RTann force-pushed the ruby-gem branch 5 times, most recently from 67c4940 to 9d44029 Compare March 21, 2023 03:42
@RTann RTann requested a review from crozzy March 21, 2023 03:54
@RTann RTann force-pushed the ruby-gem branch 7 times, most recently from 1cfca4e to e829d59 Compare March 30, 2023 22:24
Signed-off-by: RTann <rtannenb@redhat.com>
Signed-off-by: RTann <rtannenb@redhat.com>
Signed-off-by: RTann <rtannenb@redhat.com>
Signed-off-by: RTann <rtannenb@redhat.com>
Copy link
Member

@hdonnay hdonnay left a comment

Choose a reason for hiding this comment

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

This looks pending a rewrite down to a single commit

@RTann
Copy link
Contributor Author

RTann commented Apr 3, 2023

This looks pending a rewrite down to a single commit

not sure I follow?

@RTann
Copy link
Contributor Author

RTann commented Apr 3, 2023

This looks pending a rewrite down to a single commit

oh I misread at first. Thought you said "comment" not "commit". Is that needed? Wouldn't Squash and merge handle that?

@hdonnay
Copy link
Member

hdonnay commented Apr 4, 2023

oh, I suppose it would. It does skip all the commit lints though, so make sure to keep a Signed-off-by trailer and a properly formatted subject line.

@RTann RTann merged commit 14a1652 into quay:main Apr 4, 2023
@RTann RTann deleted the ruby-gem branch April 4, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants