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

move pkg, cmd & test folders in inside prow to the root of the repo #101

Merged
merged 7 commits into from
Apr 17, 2024

Conversation

upodroid
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 11, 2024
Copy link

netlify bot commented Apr 11, 2024

Deploy Preview for k8s-prow ready!

Name Link
🔨 Latest commit a9dd884
🔍 Latest deploy log https://app.netlify.com/sites/k8s-prow/deploys/661fae58f1b8d00008c95d4d
😎 Deploy Preview https://deploy-preview-101--k8s-prow.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 11, 2024
@upodroid upodroid mentioned this pull request Apr 11, 2024
@upodroid upodroid force-pushed the rearrange-repo branch 2 times, most recently from 14a86c3 to ec2a684 Compare April 11, 2024 16:27
@upodroid upodroid changed the title [WIP] move pkg and cmd to the root of the repo move pkg, cmd & test folders in inside prow to the root of the repo Apr 11, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2024
@upodroid
Copy link
Member Author

We need to merge the linked PR in test-infra to run the failing integration/image-build job.

@danilo-gemoli
Copy link
Contributor

If we want to redefine the folders structure I think we should take ghproxy into account as well. With the previous structure it was clear that Prow and GHProxy were separate as they were at the same level:

prow/
ghproxy/

Now we are going to have such a structure:

ghproxy/
cmd/
pkg/
...

that is a bit incosistent IMHO.
I'm wondering whether we should consider moving ghproxy into the cmd/ folder.

@cjwagner
Copy link
Member

I'm assuming this is following the guidelines from this page: https://go.dev/doc/modules/layout, specifically this section seems to be most applicable to the contents of our repo: https://go.dev/doc/modules/layout#packages-and-commands-in-the-same-repository

We'll also need to create an announcement and include a migration guide since this is a backwards incompatible change for consumers that depend on these go packages. Perhaps you can provide a sed command that updates import paths according to where/how they are moved by this PR?

@upodroid
Copy link
Member Author

We'll also need to create an announcement and include a migration guide since this is a backwards incompatible change for consumers that depend on these go packages.

I can send another email to the existing announcement about the updated import paths. Prow lib consumers will hopefully run into this problem +/-7 days from this comment.

I'm assuming this is following the guidelines from this page: https://go.dev/doc/modules/layout, specifically this section seems to be most applicable to the contents of our repo: https://go.dev/doc/modules/layout#packages-and-commands-in-the-same-repository

That is right, all the programs will live in cmd/* and libraries will live in pkg/*, we don't have internal libraries yet but we should start doing that soon.

I'll push a separate commit moving ghproxy to the cmd folder

Can you approve the PR in test-infra to fix the tests?

@cjwagner
Copy link
Member

I'll push a separate commit moving ghproxy to the cmd folder

Can you approve the PR in test-infra to fix the tests?

Yep, I'll approve the test-infra PR. Please don't merge it until this PR has been updated to move ghproxy as well and is also ready to merge. These PRs need to merge together so if there is a problem with either we'll need to quickly revert.

@upodroid
Copy link
Member Author

upodroid commented Apr 16, 2024

@cjwagner Image builds are hanging for some reasons, any ideas about what is happening?

@upodroid
Copy link
Member Author

This is ready to be merged.

Copy link
Contributor

@jihoon-seo jihoon-seo left a comment

Choose a reason for hiding this comment

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

@upodroid PTAL!

@@ -1,6 +1,8 @@
_bin/
_output/
.python_virtual_env
/yarn.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to remove this line, to version control the yarn.lock file;
please see yarnpkg/yarn#1583

Copy link
Member Author

@upodroid upodroid Apr 17, 2024

Choose a reason for hiding this comment

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

When we are building images or running deck locally, we use yarn to generate the node_modules and yarn.lock and run hack/ts-rollup to create the final js files that go in the container. When the build fails, we get a dirty git tree but these folders should never be committed. We are building the js code in a very unusual manner.

Copy link
Contributor

@jihoon-seo jihoon-seo Apr 17, 2024

Choose a reason for hiding this comment

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

I am not objecting to, actually am agreeing to, add 'node_modules' directory to gitignore.
What I am suggesting is that we SHOULD version control the 'yarn.lock' file, and it's just a single file, not a directory.

Oh, okay.

.gitignore Show resolved Hide resolved
@jihoon-seo
Copy link
Contributor

/label tide/merge-method-squash
(If someone feels that squash merge is not appropriate for this PR, then please feel free to remove the label.)

@k8s-ci-robot k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 17, 2024
@@ -77,6 +77,6 @@ fi
--static-files-location="$DIR/kodata/static" \
--template-files-location="$DIR/kodata/template" \
--spyglass-files-location="$DIR/kodata/lenses" \
--config-path "${DIR}/../../../config/prow/config.yaml" \
--config-path "${DIR}/../../test/integration/config/prow/deck.yaml" \
Copy link
Member Author

Choose a reason for hiding this comment

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

When deck is being developed locally, it assumes the prow config at https://github.com/kubernetes/test-infra/blob/master/config/prow/config.yaml is in the repo which is no longer true. I copied the prod config and stored it next to the integration config.yaml. I can confirm that running ./cmd/deck/runlocal runs deck locally on my laptop and it works nicely.

Comment on lines +36 to +37
cmd/ghproxy/ghproxy
pj.yaml
Copy link
Contributor

@jihoon-seo jihoon-seo Apr 17, 2024

Choose a reason for hiding this comment

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

How about adding vendor/ to .gitignore?
[Refs]

Suggested change
cmd/ghproxy/ghproxy
pj.yaml
cmd/ghproxy/ghproxy
vendor/
pj.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

Patricia has that change in #106, if we add it here she'll need to respin. Let's just merge this and then the dep bump. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, also not sure why we need to ignore vendor. I'll take a further look at 106

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't vendor in Prow so having it in gitignore helps people realize that before submitting dep bump PRs

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, upodroid

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2024
@petr-muller
Copy link
Contributor

CI changes are in place, this passes tests => :shipit:

@k8s-ci-robot k8s-ci-robot merged commit db89760 into kubernetes-sigs:main Apr 17, 2024
11 checks passed
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 17, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 17, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 17, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 19, 2024
k8s-ci-robot added a commit that referenced this pull request Apr 19, 2024
doc: Update `cmd`, `pkg`, & `test` paths (follow-up of #101)
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 20, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 20, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 20, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 30, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 30, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request Apr 30, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request May 3, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request May 3, 2024
jihoon-seo added a commit to jihoon-seo/k8s-sigs-prow that referenced this pull request May 3, 2024
k8s-ci-robot pushed a commit that referenced this pull request May 3, 2024
* Update outdated links

* Remove references to deleted tools (phaino, migratestatus)

* Update links for 'k/t-i' to 'k-s/prow'

* docs: Remove Phaino-related paragraph

* docs: Reflect code file path changes by #101

* docs: Reflect code file path changes by #101 (part2)

* docs: Reflect code file path changes by #101 (part3; ghProxy)

* Update checkconfig testcode to suppress error

* Fix errors and nits
Prucek pushed a commit to Prucek/prow that referenced this pull request May 21, 2024
Prucek pushed a commit to Prucek/prow that referenced this pull request May 21, 2024
* Update outdated links

* Remove references to deleted tools (phaino, migratestatus)

* Update links for 'k/t-i' to 'k-s/prow'

* docs: Remove Phaino-related paragraph

* docs: Reflect code file path changes by kubernetes-sigs#101

* docs: Reflect code file path changes by kubernetes-sigs#101 (part2)

* docs: Reflect code file path changes by kubernetes-sigs#101 (part3; ghProxy)

* Update checkconfig testcode to suppress error

* Fix errors and nits
Prucek pushed a commit to Prucek/prow that referenced this pull request May 21, 2024
* Update outdated links

* Remove references to deleted tools (phaino, migratestatus)

* Update links for 'k/t-i' to 'k-s/prow'

* docs: Remove Phaino-related paragraph

* docs: Reflect code file path changes by kubernetes-sigs#101

* docs: Reflect code file path changes by kubernetes-sigs#101 (part2)

* docs: Reflect code file path changes by kubernetes-sigs#101 (part3; ghProxy)

* Update checkconfig testcode to suppress error

* Fix errors and nits
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants