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 custom-resources with other examples #2157

Merged
merged 1 commit into from Nov 12, 2021

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented Nov 3, 2021

I think it makes it cleaner to have all the examples inside one folder.

@lucacome lucacome added the proposal An issue that proposes a feature request label Nov 3, 2021
@lucacome lucacome self-assigned this Nov 3, 2021
@github-actions github-actions bot added chore Pull requests for routine tasks documentation Pull requests/issues for documentation labels Nov 3, 2021
@nginx-bot nginx-bot force-pushed the chore/examples-custom-resources branch 2 times, most recently from 7d88623 to b79cd9e Compare November 4, 2021 10:41
@tomasohaodha
Copy link
Contributor

I agree with this change in principle, but would appreciate @brianehlert especially chiming in.

@pleshakov
Copy link
Contributor

custom resources is a big group of examples that covers a lot of use cases. however, with the current layout, they are somewhat hidden among Ingress resource examples:
image

imho it would be better if we have the following structure:

  • examples
    -- ingresses
    -- custom-resources

Also, perhaps we can come up with a better name than just "custom resources"? @brianehlert

@brianehlert
Copy link
Collaborator

I agree that it is valuable. And that "custom-resources" is too generic / obscure.

consumers look for answers based on the use case first and how to implement is the choice.
I don't think a large 'custom-resources' folder follows this pattern, but is rather an anti-pattern.

@lucacome lucacome force-pushed the chore/examples-custom-resources branch from b79cd9e to c3ff338 Compare November 4, 2021 20:13
@tomasohaodha
Copy link
Contributor

I would agree with all previous comments, however I would recommend approving this PR and merging the basic location change. Further changes can come in further PRs after more discussion. Perfect enemy of the good and all that.

@pleshakov
Copy link
Contributor

I don't think we should merge the current state of the PR. As I mentioned previously, the new layout hides custom resource related examples. Note that the regular and custom resource examples are supposed to be equal alternatives to each other -- that's why there were two separate folders in the root directory. With the new layout, the custom resource examples are hidden away.

@nginx-bot nginx-bot force-pushed the chore/examples-custom-resources branch 6 times, most recently from 04f4e40 to 516f17d Compare November 11, 2021 04:14
@lucacome lucacome force-pushed the chore/examples-custom-resources branch from 516f17d to 71885a6 Compare November 12, 2021 18:12
@lucacome lucacome enabled auto-merge (squash) November 12, 2021 18:15
@lucacome lucacome merged commit 3359633 into master Nov 12, 2021
@lucacome lucacome deleted the chore/examples-custom-resources branch November 12, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks documentation Pull requests/issues for documentation proposal An issue that proposes a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants