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

chore: k8s.io/kubernetes/.generated_files #3345

Closed
wants to merge 8 commits into from

Conversation

aimuz
Copy link
Collaborator

@aimuz aimuz commented Aug 24, 2023

Signed-off-by: aimuz mr.imuz@gmail.com

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Ignore generation of files

Which issue(s) this PR fixes:

Closes #

Special notes for your reviewer:

Signed-off-by: aimuz <mr.imuz@gmail.com>
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aimuz
Once this PR has been reviewed and has the lgtm label, please assign cyriltovena for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: aimuz <mr.imuz@gmail.com>
@google-oss-prow google-oss-prow bot added size/XXL and removed size/S labels Aug 24, 2023
Signed-off-by: aimuz <mr.imuz@gmail.com>
Signed-off-by: aimuz <mr.imuz@gmail.com>
Signed-off-by: aimuz <mr.imuz@gmail.com>
Signed-off-by: aimuz <mr.imuz@gmail.com>
@google-oss-prow google-oss-prow bot added size/S and removed size/XXL labels Aug 24, 2023
Signed-off-by: aimuz <mr.imuz@gmail.com>
@google-oss-prow google-oss-prow bot added size/XXL and removed size/S labels Aug 24, 2023
@aimuz
Copy link
Collaborator Author

aimuz commented Aug 24, 2023

google-oss-prow Won't read the .generated_files ?

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: b00b22a1-00e3-46f3-9c30-d77dbe9a457d

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Haven't seen this before - but one thought, not sure how much we want to be scrubbing OSS licence files from dependencies we're vendoring. Seems like bad form?

@aimuz
Copy link
Collaborator Author

aimuz commented Aug 25, 2023

Actually, what I'm trying to achieve is that the size tag calculation ignores some of the generated files. I'm not trying to delete the vendor directory.

For example, 3342 doesn't really change much code, but it has a size/XXL tag.

If the vendor directory is ignored, then the label might be size/S

This is supported by prow on k8s.

@markmandel
Copy link
Member

OIC! @roberthbailey you know PROW - any thoughts?

Copy link
Member

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand how all of the file deletions in vendor are related to the change to have prow generate change sizes differently.

If that is cleaning up dependencies, can we separate it into a different PR?

.gitattributes Outdated
@@ -14,3 +14,7 @@

# See https://github.com/golang/go/issues/9281
* -text

vendor/** linguist-generated
Copy link
Member

Choose a reason for hiding this comment

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

This line and line 20 seem redundant. Should this one be removed?

.gitattributes Outdated
@@ -14,3 +14,7 @@

# See https://github.com/golang/go/issues/9281
* -text

vendor/** linguist-generated
vendor/ linguist-generated
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 this needs to have linguist-generated=true according to https://github.com/kubernetes/test-infra/blob/master/prow/gitattributes/gitattributes.go#L89

@aimuz
Copy link
Collaborator Author

aimuz commented Aug 29, 2023

I don't want to remove the dependency, I just want to test if the settings in the generated_files file work, and if the vendor directory is ignored when calculating the size based on the generated_files file.

@aimuz
Copy link
Collaborator Author

aimuz commented Aug 29, 2023

I would like to ignore the size tag for identifying, if the current PR risk is high. If there is only a small amount of change, then the risk of this pr is considered small. If there are a lot of changes then the pr is considered to be too involved.

The files generated in this affect the calculation of the size tag.

For example, if a dependency is updated, the vendor directory will be updated to show that there are a large number of changes, and the size tag will be recorded as large.

In reality, however, it is possible that only one line in the go.mod file has been changed.

@aimuz
Copy link
Collaborator Author

aimuz commented Aug 29, 2023

The vendor directory is just an example, and there are generated files that should not be counted in the size calculation.

Signed-off-by: aimuz <mr.imuz@gmail.com>
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: e3dd2608-7f61-4235-aa9b-dc3700fe14c9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@aimuz
Copy link
Collaborator Author

aimuz commented Aug 30, 2023

It looks like setting linguist-generated=true won't work either,Are you currently using the prow version of k8s, or is there another version within your Google?

@zmerlynn
Copy link
Collaborator

zmerlynn commented Dec 4, 2023

FWIW I don't think we should take any prow config changes as I've been kind of arguing that we should just drop it. We're not using any of the features and at this point it's kind of noisy.

@markmandel
Copy link
Member

FWIW I don't think we should take any prow config changes as I've been kind of arguing that we should just drop it. We're not using any of the features and at this point it's kind of noisy.

+1 from me.

@zmerlynn
Copy link
Collaborator

zmerlynn commented Dec 5, 2023

@aimuz Thanks for the submission. We decided to eliminate Prow since it wasn't doing much for us - the webhooks are now disabled, GoogleCloudPlatform/oss-test-infra#2131 removes the config. @markmandel put up #3532 to implement the size labeler (using https://github.com/CodelyTV/pr-size-labeler), borrowed from Quilkin.

I'm going to close out this PR, but we may need to add a files_to_ignore - let's handle that later.

@zmerlynn zmerlynn closed this Dec 5, 2023
@aimuz
Copy link
Collaborator Author

aimuz commented Dec 6, 2023

If you just use prow's size function, it's really not necessary, but you always want a retest function, so that you can quickly retry if the test fails.

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

Successfully merging this pull request may close these issues.

5 participants