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: Creation of ingress resource with k8s trigger #1434

Merged

Conversation

daniel-codefresh
Copy link
Member

Signed-off-by: Daniel Soifer daniel.soifer@codefresh.io

Checklist:

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
@daniel-codefresh
Copy link
Member Author

Fixes: #1432

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

Excellent!

@whynowy whynowy merged commit 7ad6027 into argoproj:master Nov 18, 2021
@daniel-codefresh
Copy link
Member Author

@whynowy BTW, I just noticed we are using eventbus as the plural form of the EventBus CRD, is there any particular reason for this? It got me thinking that maybe we should have an optional resource field after all

names["plural"] = "eventbus"

@whynowy
Copy link
Member

whynowy commented Nov 20, 2021

@whynowy BTW, I just noticed we are using eventbus as the plural form of the EventBus CRD, is there any particular reason for this? It got me thinking that maybe we should have an optional resource field after all

names["plural"] = "eventbus"

Per @alexec (who is from England) - plural of bus is still bus...

@daniel-codefresh
Copy link
Member Author

@whynowy Oh I see.. Well I'm certainly not an English expert but from a quick Google search, it seems like buses is also an acceptable plural form of bus:
https://www.oxfordlearnersdictionaries.com/definition/english/bus_1
https://www.collinsdictionary.com/dictionary/english/bus
https://www.merriam-webster.com/words-at-play/plural-of-bus
And more importantly, I just verified that the k8s.io/gengo/namer library that was used in this PR is actually returning eventbuses as the plural form of eventbus (which will cause a failure if a user tries to CRUD an eventbus through a k8s-trigger), so I guess we would have to support that by one way or another... WDYT?

whynowy pushed a commit that referenced this pull request Nov 22, 2021
* fix: Creation of ingress resource with k8s trigger

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* used plural namer lib

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* added another test

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* codegen

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
@whynowy
Copy link
Member

whynowy commented Nov 22, 2021

@whynowy Oh I see.. Well I'm certainly not an English expert but from a quick Google search, it seems like buses is also an acceptable plural form of bus: https://www.oxfordlearnersdictionaries.com/definition/english/bus_1 https://www.collinsdictionary.com/dictionary/english/bus https://www.merriam-webster.com/words-at-play/plural-of-bus And more importantly, I just verified that the k8s.io/gengo/namer library that was used in this PR is actually returning eventbuses as the plural form of eventbus (which will cause a failure if a user tries to CRUD an eventbus through a k8s-trigger), so I guess we would have to support that by one way or another... WDYT?

I think it would be weird to support both - an extra CRD for buses. I would recommend to keep it as is. If there's really somebody want to use that to CRUD an eventbus, we should add an exception when using that lib. I remember code-nenerator used to support plural exceptions, now sure why it stopped working, and I added that workaround in the shell.

@daniel-codefresh
Copy link
Member Author

@whynowy Oh I see.. Well I'm certainly not an English expert but from a quick Google search, it seems like buses is also an acceptable plural form of bus: https://www.oxfordlearnersdictionaries.com/definition/english/bus_1 https://www.collinsdictionary.com/dictionary/english/bus https://www.merriam-webster.com/words-at-play/plural-of-bus And more importantly, I just verified that the k8s.io/gengo/namer library that was used in this PR is actually returning eventbuses as the plural form of eventbus (which will cause a failure if a user tries to CRUD an eventbus through a k8s-trigger), so I guess we would have to support that by one way or another... WDYT?

I think it would be weird to support both - an extra CRD for buses. I would recommend to keep it as is. If there's really somebody want to use that to CRUD an eventbus, we should add an exception when using that lib. I remember code-nenerator used to support plural exceptions, now sure why it stopped working, and I added that workaround in the shell.

@whynowy Yes, definitely, an extra CRD would be weird. I actually meant either adding an exception like you suggested, or changing the plural form of the existing CRD back to eventbuses so we can stick to the conventions and also get rid of that extra logic that was added to the codegen process, of transforming eventbuses to eventbus

@whynowy
Copy link
Member

whynowy commented Nov 23, 2021

@daniel-codefresh - I prefer to add exceptions, changing CRD definition sometimes brings weird issues during upgrade. Code-gen used to support exceptions, not sure why it is not supporting now. I didn't spent time on the research.

@daniel-codefresh
Copy link
Member Author

@daniel-codefresh - I prefer to add exceptions, changing CRD definition sometimes brings weird issues during upgrade. Code-gen used to support exceptions, not sure why it is not supporting now. I didn't spent time on the research.

@whynowy Oh yea, now I see what you meant by codegen exceptions.. I was thinking about using a plural exception for eventbus together with k8s.io/gengo/namer lib so we can be covered in case a user wants to CRUD eventbus resource as well. Heres the change I was talking about: #1440

BulkBeing pushed a commit to BulkBeing/argo-events that referenced this pull request Mar 7, 2022
* fix: Creation of ingress resource with k8s trigger

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* used plural namer lib

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* added another test

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* codegen

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
* fix: Creation of ingress resource with k8s trigger

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* used plural namer lib

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* added another test

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>

* codegen

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
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.

2 participants