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(executor): Resource template gets incorrect plural for certain types #9396

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

wreed4
Copy link
Contributor

@wreed4 wreed4 commented Aug 19, 2022

Fixes #9393

@wreed4 wreed4 force-pushed the fix-plural-name-bug branch from 268834f to f9753bc Compare August 19, 2022 11:59
@wreed4 wreed4 changed the title fix plural name bug Fixes #9393 Aug 19, 2022
@wreed4 wreed4 force-pushed the fix-plural-name-bug branch from f9753bc to 3de4e27 Compare August 19, 2022 12:09
Signed-off-by: William Reed <[email protected]>
@terrytangyuan terrytangyuan self-assigned this Aug 19, 2022
@terrytangyuan terrytangyuan changed the title Fixes #9393 fix(executor): resource template gets incorrect plural for certain types Aug 19, 2022
@terrytangyuan terrytangyuan changed the title fix(executor): resource template gets incorrect plural for certain types fix(executor): Resource template gets incorrect plural for certain types Aug 19, 2022
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thank you! Since we only rely on a single function from gengo, it might be better to fork the implementation (with a link and credit to original source code) instead of adding a less popular dependency.

@@ -77,7 +78,11 @@ func inferObjectSelfLink(obj unstructured.Unstructured) string {
gvk := obj.GroupVersionKind()
// This is the best guess we can do here and is what `kubectl` uses under the hood. Hopefully future versions of the
// REST client would remove the need to infer the plural name.
pluralGVR, _ := meta.UnsafeGuessKindToResource(gvk)
Copy link
Member

@terrytangyuan terrytangyuan Aug 19, 2022

Choose a reason for hiding this comment

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

Can you check if there’s any improvement to this function in the new versions of meta? Maybe the issue is already fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the PR that is open to fix the issue, but it's not merged and it's unclear to me if people are happy with this solution or if they want to use the solution I did in this PR.

kubernetes/kubernetes#110053

In master it's still in silly mode https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/meta/restmapper.go#L143

@wreed4
Copy link
Contributor Author

wreed4 commented Aug 19, 2022

re: forking the implementation it's not terribly much to fork and with a comment maybe it's okay, but using this function directly was suggested in the client-go bug because this is what is used by the tools that generate things like DeepCopy in controller-gen and a lot of other boiler plate code. Personally I wouldn't want to split away from that logic, but I do understand it's a small bit of code to add a whole dependency for. So whichever you guys feel happier with I'm happy to do.

This is the function implementation just to make things simple. There are a handful of calls out, but it's not massive. It is more than I would want to copy and paste personally, but again up to you guys.

// Name returns the plural form of the type's name. If the type's name is found
// in the exceptions map, the map value is returned.
func (r *pluralNamer) Name(t *types.Type) string {
	singular := t.Name.Name
	var plural string
	var ok bool
	if plural, ok = r.exceptions[singular]; ok {
		return r.finalize(plural)
	}
	if len(singular) < 2 {
		return r.finalize(singular)
	}

	switch rune(singular[len(singular)-1]) {
	case 's', 'x', 'z':
		plural = esPlural(singular)
	case 'y':
		sl := rune(singular[len(singular)-2])
		if isConsonant(sl) {
			plural = iesPlural(singular)
		} else {
			plural = sPlural(singular)
		}
	case 'h':
		sl := rune(singular[len(singular)-2])
		if sl == 'c' || sl == 's' {
			plural = esPlural(singular)
		} else {
			plural = sPlural(singular)
		}
	case 'e':
		sl := rune(singular[len(singular)-2])
		if sl == 'f' {
			plural = vesPlural(singular[:len(singular)-1])
		} else {
			plural = sPlural(singular)
		}
	case 'f':
		plural = vesPlural(singular)
	default:
		plural = sPlural(singular)
	}
	return r.finalize(plural)
}

@wreed4 wreed4 marked this pull request as ready for review August 19, 2022 20:15
@terrytangyuan
Copy link
Member

terrytangyuan commented Aug 23, 2022

this is what is used by the tools that generate things like DeepCopy in controller-gen and a lot of other boiler plate code

Could you link me to where this is being used?

Another concern is that there isn't any stable releases yet: https://github.com/kubernetes/gengo/releases

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@terrytangyuan terrytangyuan merged commit de6b5ae into argoproj:master Aug 25, 2022
@wreed4 wreed4 deleted the fix-plural-name-bug branch August 26, 2022 15:26
juchaosong pushed a commit to juchaosong/argo-workflows that referenced this pull request Nov 3, 2022
…pes (argoproj#9396)

* fix(executor): resource template gets incorrect plural for certain types

Signed-off-by: William Reed <[email protected]>

* fix(executor): fix lint

Signed-off-by: William Reed <[email protected]>

Signed-off-by: William Reed <[email protected]>
Signed-off-by: juchao <[email protected]>
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.

Some resources cannot use success/failure conditions at all due to bug in UnsafeGuessKindToResource
2 participants