Automatically move generated client-go files to repo root #221
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielvegamyhre 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 |
k8s-ci-robot
added
approved
k8s-ci-robot
added
size/S
|
/retest |
danielvegamyhre
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@kannon92 Can you checkout this PR locally, try generating the client-go files, and let me know if the fix works? It works for me locally when I clone jobset into /tmp folder, delete |
Doing this now! |
|
|
||
| bash "${CODEGEN_PKG}/generate-groups.sh" \ | ||
| "all" \ | ||
| sigs.k8s.io/jobset/client-go \ | ||
| sigs.k8s.io/jobset/api \ | ||
| jobset:v1alpha2 \ | ||
| --go-header-file ./boilerplate.go.txt | ||
|
|
||
| # if client-go files were generated outside of repo root, attempt to move them to the repo root. |
There was a problem hiding this comment.
Testing this, it works but you will have to delete your client-go directory in order to get the copy to work. its a bit odd.
Can we just overwrite if we assume that these changes will never be modified manually?
I deleted everything under client-go directory but I reread your script and it wouldn't copy those changes so I had to delete the client-go directory also.
Maybe we just assume anything in client-go can be purged if you are running this?
There was a problem hiding this comment.
I added a force flag to the mv command so that it will overwrite if necessary, so now you shouldn't need to manually delete client-go/, can you try it again now? I think it's fine to assume the generated files will be overwritten.
There was a problem hiding this comment.
The issue is this code:
if [ ! -d "$REPO_ROOT/client-go" ]; then
This is saying that if the folder exists then we won't copy it.
Maybe we just remove this if check?
There was a problem hiding this comment.
Oh I see what you mean now - thanks! I removed this check and also added a check before moving the files to only move it if it isn't already in the repo root (since if that is the case, the mv command will fail even with the force flag).
|
/lgtm Thanks for fixing this! |
k8s-ci-robot
added
the
lgtm
Fixes #219