SYSENG-1357: generic client default options #361

ghost · 2024-04-24T05:31:10Z

Description

This PR adds functionality to define default requests options via WithRequestOptions when creating a new generic API client.

Checklist

added release notes to Unreleased section in CHANGELOG.md, if user facing change

References

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

codeclimate · 2024-04-24T05:54:47Z

Code Climate has analyzed commit f5b16ea and detected 0 issues on this pull request.

View more on Code Climate.

pkg/apis/kubernetes/v1/common.go

pkg/api/options.go

89Q12

LGTM

LittleFox94

found a few things that would be worth changing, but please also first note what I wrote you in Mattermost

LittleFox94 · 2024-04-29T12:07:06Z

pkg/api/types/options.go

+type AnyOption interface {
+	ApplyToAny(Options) error
+}
+
+type Option interface{}
+


missing doc comments

LittleFox94 · 2024-04-29T12:08:26Z

pkg/apis/kubernetes/v1/environments.go

+	"go.anx.io/go-anxcloud/pkg/api/types"
+)
+
+func EnvironmentProduction(override bool) types.Option {


missing doc comment

Added the comment ~~(not pushed yet)~~.

But do we need this file and its content at all in this project? We are already defaulting to the prod environment.

hm probably not, good point - up to you

LittleFox94 · 2024-04-29T12:09:24Z

pkg/apis/kubernetes/v1/node_pool_test.go

+		url, err := np.EndpointURL(
+			types.ContextWithOperation(types.ContextWithOptions(context.TODO(), &types.ListOptions{}), types.OperationList))


why is this change necessary?

See comment below

LittleFox94 · 2024-04-29T12:11:56Z

pkg/api/options.go

+	if options, err := types.OptionsFromContext(ctx); err != nil {
+		return "", err


shouldn't it just return the default value in this case?

Hmm.. not sure about that. I think Options are always expected to be available on the context when using this helper (GetEnvironmentPathSegment) defaulting to prod (in most cases) on error sounds a bit dangerous.

really not sure .. I think the risk to default to prod due to this is pretty low and can be reasonably enough tested with unit tests, but makes the code using this function a lot nicer in exchange

And dealing with the risk is also possible by using tokens that cannot use the wrong service

LittleFox94 · 2024-04-29T12:14:36Z

pkg/api/options.go

+	} else if err != nil {
+		return "", err
+	} else if envString, ok := env.(string); !ok {
+		return "", ErrEnvironmentSegmentNotTypeString


if the comment above is accepted, this is the last branch that could return an error - could it be sensible to have a environments variable on types.commonOptions that can only hold strings, removing the need for this function to be able to return an error, which then also helps with the now 5 returns in apis/kubernetes/v1.endpointURL?

With extra methods SetEnvironment & GetEnvironment on types.Options i suppose?

something like that, yea

pkg/api/api_implementation.go

LittleFox94 · 2024-05-08T12:43:46Z

pkg/api/api_implementation.go

+	for _, requestOpt := range a.requestOptions {
+		if getOpt, ok := requestOpt.(types.GetOption); ok {
+			err = errors.Join(err, getOpt.ApplyToGet(&options))
+		}
+	}
 	for _, opt := range opts {
-		opt.ApplyToGet(&options)
+		err = errors.Join(err, opt.ApplyToGet(&options))
+	}


this can be prettied up a bit for all the request methods with a small generic helper:

func resolveRequestOptions[T any](commonOptions []types.Option, requestOptions []T) []T { return append(filterOptions[T](commonOptions), requestOptions...) } func filterOptions[T any](opts []types.Option) []T { ret := make([]T, 0, len(opts)) for _, v := range opts { if v, ok := v.(T); ok { ret = append(ret, v) } } return ret }

With that present, the marked code would be replaced with this:

for _, opt := range resolveRequestOptions(a.requestOptions, opts) { err = errors.Join(err, opt.ApplyToGet(&options)) }

LittleFox94 · 2024-05-08T12:44:54Z

pkg/api/types/options.go

@@ -73,3 +107,26 @@ func (o *commonOptions) Set(key string, val interface{}, overwrite bool) error {
 	o.additional[key] = val
 	return nil
 }
+
+func (o commonOptions) GetEnvironment(key string) (string, error) {


no doc comment..

LittleFox94 · 2024-05-08T12:45:13Z

pkg/api/errors.go

+
+	// ErrEnvironmentSegmentNotTypeString is returned when the environment segment is not of type string
+	ErrEnvironmentSegmentNotTypeString = errors.New("environment segment is not of type string")


not needed anymore, right?

89Q12

LGTM, the failing integration tests well meh.
Otherwise doc strings have all been added.

LittleFox94 · 2024-05-17T09:42:23Z

All relevant integration tests went through, I'm ok with merging without all of them being green

ghost self-assigned this Apr 24, 2024

ghost force-pushed the SYSENG-1357/generic-client-default-options branch from 9ead8f6 to 3f68833 Compare April 24, 2024 05:54

codeclimate bot reviewed Apr 24, 2024

View reviewed changes

pkg/apis/kubernetes/v1/common.go Show resolved Hide resolved

pkg/api/options.go Outdated Show resolved Hide resolved

ghost force-pushed the SYSENG-1357/generic-client-default-options branch 2 times, most recently from 3a1abc3 to 167a9f9 Compare April 24, 2024 06:19

ghost marked this pull request as ready for review April 24, 2024 06:23

89Q12 approved these changes Apr 24, 2024

View reviewed changes

LittleFox94 requested changes Apr 29, 2024

View reviewed changes

ghost force-pushed the SYSENG-1357/generic-client-default-options branch from b1074c9 to e6d05f2 Compare May 3, 2024 04:52

LittleFox94 reviewed May 8, 2024

View reviewed changes

ghost force-pushed the SYSENG-1357/generic-client-default-options branch 3 times, most recently from d56adca to 9fc6738 Compare May 13, 2024 10:18

ghost added the integration_tests Used to approve the run of integration tests! label May 13, 2024

SYSENG-1357: generic client default options

512b567

ghost force-pushed the SYSENG-1357/generic-client-default-options branch from 9fc6738 to 512b567 Compare May 13, 2024 10:48

ghost added integration_tests Used to approve the run of integration tests! and removed integration_tests Used to approve the run of integration tests! labels May 13, 2024

Mario Schäfer added 2 commits May 14, 2024 11:45

CI: Run integration tests only for 'target' go version

d6b1011

Increase VM provisioning timeout in tests

f5b16ea

ghost added integration_tests Used to approve the run of integration tests! and removed integration_tests Used to approve the run of integration tests! labels May 14, 2024

89Q12 reviewed May 16, 2024

View reviewed changes

ghost merged commit bf9a6d6 into main May 17, 2024
16 of 17 checks passed

ghost deleted the SYSENG-1357/generic-client-default-options branch May 17, 2024 09:43

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SYSENG-1357: generic client default options #361

SYSENG-1357: generic client default options #361

ghost commented Apr 24, 2024

codeclimate bot commented Apr 24, 2024 •

edited

Loading

89Q12 left a comment

LittleFox94 left a comment

LittleFox94 Apr 29, 2024

LittleFox94 Apr 29, 2024

ghost Apr 30, 2024 •

edited by ghost

Loading

LittleFox94 Apr 30, 2024

LittleFox94 Apr 29, 2024

ghost Apr 30, 2024

LittleFox94 Apr 29, 2024

ghost Apr 30, 2024

LittleFox94 Apr 30, 2024

LittleFox94 Apr 29, 2024

ghost Apr 30, 2024

LittleFox94 Apr 30, 2024

LittleFox94 May 8, 2024

LittleFox94 May 8, 2024

LittleFox94 May 8, 2024

89Q12 left a comment •

edited

Loading

LittleFox94 commented May 17, 2024

		url, err := np.EndpointURL(
		types.ContextWithOperation(types.ContextWithOptions(context.TODO(), &types.ListOptions{}), types.OperationList))

		if options, err := types.OptionsFromContext(ctx); err != nil {
		return "", err


		// ErrEnvironmentSegmentNotTypeString is returned when the environment segment is not of type string
		ErrEnvironmentSegmentNotTypeString = errors.New("environment segment is not of type string")

SYSENG-1357: generic client default options #361

SYSENG-1357: generic client default options #361

Conversation

ghost commented Apr 24, 2024

Description

Checklist

References

Community Note

codeclimate bot commented Apr 24, 2024 • edited Loading

89Q12 left a comment

Choose a reason for hiding this comment

LittleFox94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 30, 2024 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

89Q12 left a comment • edited Loading

Choose a reason for hiding this comment

LittleFox94 commented May 17, 2024

codeclimate bot commented Apr 24, 2024 •

edited

Loading

ghost Apr 30, 2024 •

edited by ghost

Loading

89Q12 left a comment •

edited

Loading