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

Standard Service configuration #7

Closed
wants to merge 2 commits into from
Closed

Conversation

eccles
Copy link
Member

@eccles eccles commented Sep 14, 2023

Make the tenancies client of the accesspolicies service use new
standard configuration.

Added service configuration logic to startup package using idiomatic
Go With...() pattern..

Part of AB#8170

 Mention [stepsize] in a comment if you'd like to report some technical debt. See examples here.

@eccles eccles marked this pull request as draft September 14, 2023 14:00
@@ -0,0 +1,123 @@
package grpcclient

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the metadata constants and some of the functions are not naturaly part of the client module

I have a middleware package in my branch of go-rkvstcommon for things like this

https://github.com/rkvst/go-rkvstcommon/tree/dev/robin/8337-common-metadata/middleware

I think grpcclient is fine in general for things that are definitely only the concern of client users and implementors

var (
ErrNotFound = errors.New("Environment variable not found")
)

// GetLogLevel returns the loglevet or panics. This is called before any logger
// is available. i.e. don't use a logger here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have previously established the OrFatal convention because some people object to functions which terminate. I understand we cant use logger.Fatalf here because this must be called before it is created. I think OrPanic would be better aligned with established patterns.

Personally, I have no problem with declaring that the environment module is 'allowed to panic or fatal out' because it is designed to be used as part of pre-flight startup. And then remove all the OrWhatever suffixes.

But we should get everyone on board with that before doing so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is a speculative change only and may go away...

startup/grpcclient.go Show resolved Hide resolved
Paul Hewlett added 2 commits November 9, 2023 15:00
Make the tenancies client of the accesspolicies service use new
standard configuration.

Added service configuration logic to startup package using idiomatic
Go With...() pattern..

Part of AB#8170
@eccles eccles force-pushed the dev/paul/8170-tenencies-client branch from 4512e28 to fdbfccf Compare November 9, 2023 15:01
@eccles eccles closed this Feb 7, 2024
@eccles
Copy link
Member Author

eccles commented Feb 7, 2024

obsolete

@eccles eccles deleted the dev/paul/8170-tenencies-client branch February 7, 2024 11:42
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