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

CLOUDP-293822: Fix targets affected by new project layout #2028

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

helderjs
Copy link
Collaborator

@helderjs helderjs commented Jan 9, 2025

All Submissions:

  • Have you signed our CLA?
  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if there is one).
  • Update docs/release-notes/release-notes-template.md if your changes should be included in the release notes for the next release.

Copy link
Collaborator

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

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

@helderjs Looks good, but I think the "scripts/deploy.sh" also has to be updated

Copy link
Collaborator

@josvazg josvazg left a comment

Choose a reason for hiding this comment

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

LGTM, so far, just holding in case more changes are also needed as mentioned by @igor-karpukhin

@igor-karpukhin
Copy link
Collaborator

@helderjs one more place I found is here:

controller-gen crd:crdVersions=v1,ignoreUnexportedFields=true rbac:roleName=manager-role webhook paths="./internal/..." output:crd:artifacts:config=config/crd/bases

@roothorp
Copy link
Collaborator

roothorp commented Jan 9, 2025

Does this mean we have been failing to generate/update some things since we changed the project structure?

@helderjs
Copy link
Collaborator Author

helderjs commented Jan 9, 2025

Does this mean we have been failing to generate/update some things since we changed the project structure?

Probably, but the PR was merged Monday, so previous deliveries were not affected.

@roothorp
Copy link
Collaborator

roothorp commented Jan 9, 2025

Probably, but the PR was merged Monday, so previous deliveries were not affected.

Oh, for some reason I thought we'd made this change earlier than that! In that case LGTM.

@helderjs helderjs merged commit 3c9074e into main Jan 9, 2025
9 checks passed
@helderjs helderjs deleted the fix-makefile-targets branch January 9, 2025 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants