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

Case convention inconsistent in Knative Serving config maps #7300

Closed
mgencur opened this issue Mar 18, 2020 · 12 comments
Closed

Case convention inconsistent in Knative Serving config maps #7300

mgencur opened this issue Mar 18, 2020 · 12 comments
Assignees
Labels
area/API API objects and controllers kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@mgencur
Copy link
Contributor

mgencur commented Mar 18, 2020

Some config maps have keys using camel-case, some have kebab-case. The majority of the config maps use camel-case at this point. All of this should probably be unified.

camel-case used in these config maps:

  • config-deployment
  • config-leader-election
  • config-network

Expected Behavior

All config maps using the same notation.

Actual Behavior

Some config maps using a different notation than others.

Steps to Reproduce the Problem

@mgencur mgencur added the kind/bug Categorizes issue or PR as related to a bug. label Mar 18, 2020
@vagababov
Copy link
Contributor

ugh, that ship has sailed probably... otherwise it's a long and painful migration. :|

@julz
Copy link
Member

julz commented Jun 9, 2020

I wonder if now that we have the nice config map parsing library in pkg we could make this change in a backwards compatible way without a completely unreasonable amount of pain 🤔. It's only going to worse if we don't so I vote we bite the bullet, fwiw.

Hyphens ftw ;).

@mattmoor
Copy link
Member

Another alternative is @markusthoemmes proposal to switch to CRDs. Care to link that?

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 16, 2020
@mattmoor
Copy link
Member

/lifecycle frozen

Ping @markusthoemmes for link

@knative-prow-robot knative-prow-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 16, 2020
@markusthoemmes
Copy link
Contributor

#8114 here's the other issue.

@evankanderson
Copy link
Member

I think #8114 is probably the best chance to resolve this issue.

/area API

/close

@knative-prow-robot knative-prow-robot added the area/API API objects and controllers label Mar 22, 2021
@knative-prow-robot
Copy link
Contributor

@evankanderson: Closing this issue.

In response to this:

I think #8114 is probably the best chance to resolve this issue.

/area API

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dprotaso
Copy link
Member

if now that we have the nice config map parsing library in pkg we could make this change in a backwards compatible way without a completely unreasonable amount of pain 🤔. It's only going to worse if we don't

I think for the 1.0 efforts we should do this and keeping the backwards compatible

/reopen
/assign @dprotaso

@knative-prow-robot
Copy link
Contributor

@dprotaso: Reopened this issue.

In response to this:

if now that we have the nice config map parsing library in pkg we could make this change in a backwards compatible way without a completely unreasonable amount of pain 🤔. It's only going to worse if we don't

I think for the 1.0 efforts we should do this and keeping the backwards compatible

/reopen
/assign @dprotaso

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dprotaso added a commit to dprotaso/pkg that referenced this issue Sep 30, 2021
Part of knative/serving#7300

Majority of serving uses lowercase-hyphens - ie. some-property over
someProperty

This changes the leader election config map parsing to support formats
and to prioritize the lowercase-hyphen case
knative-prow-robot pushed a commit to knative/pkg that referenced this issue Oct 6, 2021
Part of knative/serving#7300

Majority of serving uses lowercase-hyphens - ie. some-property over
someProperty

This changes the leader election config map parsing to support formats
and to prioritize the lowercase-hyphen case
@cardil
Copy link

cardil commented Oct 7, 2021

I've just filled a similar issue to track label and annotation names as well - #12103

I think, those two should be done in synchronization to lower the migration pain for end users.

@dprotaso
Copy link
Member

I think all the config maps are sorted and the documentation was the last remaining bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests

9 participants