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

fix: unable to build multigroup projects #1507

Merged
merged 1 commit into from
May 29, 2020

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented May 8, 2020

closes: #1500

PS.: I do not consider this bug fix as a braking change because:

After having project scaffold and with the project layout update to multigroup support then, users are still able to use the command to re-run the edit --multigroup=true which would apply the fix.

ps.: shows that we have not too many users using the multigroup since the issue was not reported so far.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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 requested review from Adirio and droot May 8, 2020 13:26
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 8, 2020
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 8, 2020
@camilamacedo86
Copy link
Member Author

Hi @estroz @mengqiy @droot could you help by checking this one?

@camilamacedo86

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 8, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 8, 2020
@camilamacedo86

This comment has been minimized.

@camilamacedo86 camilamacedo86 changed the title fix: any layout should keep api/ as dir WIP: fix: any layout should keep api/ as dir May 8, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2020
@estroz
Copy link
Contributor

estroz commented May 8, 2020

Instead of breaking scraffolded directory structure, something kubebuilder should do as infrequently as possible, why not add a sentence in the multi group migration docs to update the Dockerfile?

@camilamacedo86
Copy link
Member Author

HI @estroz,

Thank you for your input. However, I do no thinking that we should follow-up with your suggestion.

  1. Why change the api to apis when is multi-group? It has no relation with so we should not.
  2. Add add a sentence in the multi-group migration docs to update the Dockerfile? it would be possible, however, may have other places that were affected and are facing issues too. Note that we did not get the issue before which shows that users are not really using it yet. And then, I do not think that request these unnecessary manually changes are a good user experience at all.

@camilamacedo86

This comment has been minimized.

@estroz
Copy link
Contributor

estroz commented May 8, 2020

My 2 cents:

  1. A project fundamentally changes when it transitions to support multiple groups, which is probably why the directory structure changes and should change.
  2. I have not seen any related issues aside from Build is not working for multi-group #1500.

This change sounds more like a matter of preference than one that should happen for a good technical reason, since we can easily document the Dockerfile change like all other multigroup migration changes. Perhaps a better approach would be to write a phase 2 plugin to support this preference, like the one suggested in #932.

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented May 8, 2020

HI @estroz,

I am not sure if it is a matter of the preference since it is broking the project and do not let the users run the testadata gen with multigroup as suggested in the docs. When we added the multi-group option we decided to add if is multi-group the use apis/ instead of api/ or in the cleanup and changes made in this area after that, however, IHMO it shows that was a wrong decision.

So, IHMO technically in order to keep it simple and avoid other possible issues we do not need really change the name of the dir api when is multigroup since it has no relation with. Or increase the complexity of users work with and perform manual changes.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2020
@camilamacedo86
Copy link
Member Author

/hold

@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 May 8, 2020
@camilamacedo86

This comment has been minimized.

@camilamacedo86 camilamacedo86 reopened this May 8, 2020
@camilamacedo86 camilamacedo86 changed the title WIP: fix: any layout should keep api/ as dir fix: unable to build multigroup projects May 8, 2020
@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 May 8, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2020
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented May 8, 2020

Hi @estroz,

After the discussion, the solution was changed to just provide the fix.

@camilamacedo86
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 8, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 8, 2020
@camilamacedo86

This comment has been minimized.

@camilamacedo86

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2020
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-14-1

Copy link

@NicolasT NicolasT left a comment

Choose a reason for hiding this comment

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

Ran into this as well, suggested change is what I applied manually to resolve the issue.

Copy link

@djzager djzager left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit 216d8c9 into kubernetes-sigs:master May 29, 2020
@camilamacedo86 camilamacedo86 deleted the fix-build branch May 29, 2020 18:29
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/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.

Build is not working for multi-group
5 participants