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

Add config option to remove .gitattributes files #285

Closed
wants to merge 1 commit into from

Conversation

nikhita
Copy link
Member

@nikhita nikhita commented Jan 17, 2022

If a staging repo has .gitattributes files containing the export-subst
attribute (example), then git expands the specified placeholders
when git archive is used.

When a published repo is downloaded from GitHub, GitHub does a
"git archive" under the hood. This means that the placeholders get
replaced by their relevant values. This type of "git archive"
application sometimes leads to undesired values. See the example below.

  • In client-go, line 59 in pkg/version/base.go
    is expanded on a git archive. This line is needed as a fallback to
    inject k8s version info for client-go when this info is not provided
    via ldflags during builds.

  • However, when k/client-go is vendored, the line gets expanded to the
    commit of the project vendoring client-go
    -- which is not helpful at
    all! This also means that the vendored client-go will now contain
    different (expanded) commit shas for different projects.

  • To ensure reproducibility of source, this commit adds an option to
    remove the .gitattributes files in all published repos.
    Additionally, when client-go is used as a library, we don't care about
    the line being expanded to inject version info so it is also safe to
    remove these files.

The commit adds this feature as a config option in rules.yaml so we
can control publishing of .gitattributes files for each repo.


Ref:


I have tested this on master for client-go and component-base. The result will look something like this for:

  1. client-go - https://github.com/prowtest/client-go/commits/master
  2. component-base - https://github.com/prowtest/component-base/commits/master

/hold
Adding a hold because I'd like to ensure tags work fine too. Will do that later this week but wanted to get the PR out for review.

/assign @sttts @dims

If a staging repo has .gitattributes files containing the `export-subst`
attribute (example: https://github.com/kubernetes/kubernetes/blob/b6c06a95d7ff262a876b95a1035eaf478a7cb961/staging/src/k8s.io/client-go/pkg/version/.gitattributes),
then git expands the specified placeholders when git archive is used.

When a published repo is downloaded from GitHub, GitHub does a
"git archive" under the hood. This means that the placeholders get
replaced by their relevant values. This type of "git archive"
application sometimes leads to undesired values. See the example below.

- In client-go, https://github.com/kubernetes/kubernetes/blob/b6c06a95d7ff262a876b95a1035eaf478a7cb961/staging/src/k8s.io/client-go/pkg/version/base.go#L59
is expanded on a git archive. This line is needed as a fallback to
inject k8s version info for client-go when this info is not provided
via ldflags during builds.

- However, when k/client-go is vendored, the line gets expanded to _the
commit of the project vendoring client-go_ -- which is not helpful at
all! This also means that the vendored client-go will now contain
different (expanded) commit shas for different projects.

- To ensure reproducibility of source, this commit adds an option to
remove the `.gitattributes` files in all published repos.
Additionally, when client-go is used as a library, we don't care about
the line being expanded to inject version info so it is also safe to
remove these files.

The commit adds this feature as a config option in `rules.yaml` so we
can control publishing of `.gitattributes` files for each repo.
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 17, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhita

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

The pull request process is described 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

@dims
Copy link
Member

dims commented Jan 17, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2022
@dims
Copy link
Member

dims commented Jan 17, 2022

@kubernetes/release-engineering please review!

if [ ! $# -eq 14 ]; then
echo "usage: $0 repo src_branch dst_branch dependent_k8s.io_repos required_packages kubernetes_remote subdirectory source_repo_org source_repo_name base_package is_library recursive_delete_pattern skip_tags last_published_upstream_hash"
if [ ! $# -eq 15 ]; then
echo "usage: $0 repo src_branch dst_branch dependent_k8s.io_repos required_packages kubernetes_remote subdirectory source_repo_org source_repo_name base_package is_library recursive_delete_pattern skip_tags last_published_upstream_hash remove_git_attributes"
Copy link
Member

Choose a reason for hiding this comment

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

does recursive_delete_pattern not already support this?

Copy link
Member

Choose a reason for hiding this comment

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

@nikhita ping!

@nikhita
Copy link
Member Author

nikhita commented Mar 24, 2022

Closing in favor of kubernetes/kubernetes#108970

@nikhita nikhita closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants