cleanup: Use const values to handle layout / entries well known values and generate strings instead of hardcoding them #626
Conversation
3v1n0
force-pushed
the
common-consts
branch
2 times, most recently
from
c6d87de
to
be360ff
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
+ Coverage 83.20% 83.27% +0.07%
==========================================
Files 80 82 +2
Lines 8617 8702 +85
Branches 75 75
==========================================
+ Hits 7170 7247 +77
- Misses 1115 1120 +5
- Partials 332 335 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Yeah, I'm glad you think the same. I had it in the works for a while, maybe we should find a clean way to expose the shared ones with the brokers too, but for now even copying this is fine probably 😅 |
I was wondering if there is a reason why the broker doesn't depend on authd currently. To me it makes sense to share the constants between authd and the broker. @didrocks, WDYT? |
Yeah, ideally we should have an intermediate library that provides the shared elements that can be then used both by the example broker and any other go broker out there that we've now and that may show up in future |
Depending on the amount of elements we share. If those are only a few constants, maybe keeping up to date |
IIUC, these are constants which need to be in sync between authd and the broker, so the alternative is that we update them in both repos every time we change something there. I expect that to be more time consuming than only updating them in authd and then running |
A mono repo would make this even simpler of course 😁 |
Let’s go with this then! Just try to expose as public only the needed bits please! |
Want to handle it in a different PR or here? |
As you are adding the consts here, let’s expose them directly publicly |
|
Ok... Will move them. Ideally I would have used a different strategy though: we could have used proto files enums and expose them instead (after a rename) and use the same strategy to generate the JSON that is passed through dbus too (more or less how it happens with gdm), as it would have saved us lots of manual management of the types and the usage of But I'm unsure if that's too late now. |
3v1n0
force-pushed
the
common-consts
branch
4 times, most recently
from
7ad89c7
to
26c1995
Compare
3v1n0
force-pushed
the
common-consts
branch
4 times, most recently
from
6f91837
to
24ae66f
Compare
|
@adombeck so things are exported now and I've take the opportunity to make possible to use more structured data on broker side, so please check this again together with ubuntu/authd-oidc-brokers#234 |
Avoid building the list manually now that we've consts values and use this for both optional booleans and supported entries
It can be used to parse optional or required values or entries. Use this both in broker and example brokers logic
Avoid repeating the same string allover the places
We were using a maps of maps that made code hard to read and maintain and doing some unneeded conversions to json back and forth. Avoid this by just using structured data instead
So that we can reuse their ID across the file without further constants
We don't need to expose the proto externally, since we are now re-usable so if something of it has to be exposed we should do it manually
It's what the oidc broker expects to be set in case the rendering is not supported, while we were unsetting the value by default.
…se them We use to create UI options manually and converting them to maps for being transferred via DBus, it's something we also need to do in the broker side, so let's expose this publicly for being reused by brokers too
If some values are unset, there's no point to expose them to the brokers
Now that it has been tested with the manually generated version we can use the utility functions.
It can be useful for brokers to handle the received messages
…layouts It adds a convenient way to handle the value as a native type.
These can be used by brokers and authd to parse the results back and forth from the dbus format.
Now that authd can be imported we should not expose anything that isn't an API.
| @@ -0,0 +1,25 @@ | |||
| // Package auth contains the authentication related code. | |||
| package auth | |||
There was a problem hiding this comment.
What do you think about moving the the auth and layout packages from brokers/ to pkg/? I find it a bit odd that these packages are below a brokers directory, because the way I see it, these are authd's authentication modes and layouts, not the brokers' - the brokers only choose which of those modes and layouts they support.
Putting packages meant to be imported by other projects below pkg/ is a common pattern in Go projects and IMO brings some structure into our already quite chaotic project structure.
There was a problem hiding this comment.
pkg/ while very popular in the past, is an old idiom in Go. It’s clearly not seen as a best practice anymore since around 2018. Brad Fitzpatrick has stated that they stopped using it in the stdlib a long time ago. The conscensus on the popular go project standard project (not official) is to not use it either.
Basically the current stance that I saw many times in conferences and podcasts are:
- everything private is in
internal - everything else is public and should be at the root of the project (if possible, like cobra) and can be separated by namespaces (subfolders like golang x/net package).
It’s also the evolution we followed: ubuntu-report had a pkg/ subfolder and never evolved: https://github.com/ubuntu/ubuntu-report while our newer projects don’t.
So I will let you handle the renaming as you want and I think most of what you want to expose should be at to the root of the project. However please, refrain to use a pkg/ subfolder.
There was a problem hiding this comment.
pkg/ while very popular in the past, is an old idiom in Go. It’s clearly not seen as a best practice anymore since around 2018. Brad Fitzpatrick has stated that they stopped using it in the stdlib a long time ago. The conscensus on the popular go project standard project (not official) is to not use it either.
Well, that issue contains a lengthy discussion, was never resolved and the project layout still contains pkg, so it's just not true that there is consensus on that.
However, I've also observed the trend that it's used less than it was, and for Go projects which primarily contain packages intended to be used in other projects (like the stdlib), I agree that it does not add any value and just convolutes the import path. I would argue that things are different in more complex projects like authd, which are not primarily intended as a library but want to publish only a few packages for other projects that want to use authd, then it makes sense to put those packages in a directory which makes that purpose clear, and pkg/ still serves that purpose. I don't care enough to continue arguing for it though. Maybe we can find a different name than brokers/ or pkg/ to put those packages in?
There was a problem hiding this comment.
Well, that issue contains a lengthy discussion, was never resolved and the project layout still contains pkg, so it's just not true that there is consensus on that.Well, that issue contains a lengthy discussion, was never resolved and the project layout still contains pkg, so it's just not true that there is consensus on that.
Nor than "Putting packages meant to be imported by other projects below pkg/ is a common pattern in Go projects " is true nowday.
To move this forward, what about just moving them to the root of authd? How many do we envision having and do they need separate namespacing?
| type InvalidModeError struct { | ||
| error | ||
| } |
There was a problem hiding this comment.
There is no reason to wrap an error inside this. It's more idiomatic to just have the message as the field, and implement Error() and New...() functions.
| type InvalidModeError struct { | |
| error | |
| } | |
| type InvalidModeError struct { | |
| Message string | |
| } | |
| // Error is the implementation of the error interface. | |
| func (e InvalidModeError) Error() string { | |
| return e.Message | |
| } | |
| // NewInvalidModeError returns a new InvalidModeError. | |
| func NewInvalidModeError(message string) error { | |
| return InvalidModeError{Message: message} | |
| } |
| // UITypeError defines an error for [UIType] errors. | ||
| type UITypeError struct { | ||
| error | ||
| } |
There was a problem hiding this comment.
Same here, we don't need to wrap an error.
| @@ -0,0 +1,25 @@ | |||
| // Package auth contains the authentication related code. | |||
| package auth | |||
There was a problem hiding this comment.
pkg/ while very popular in the past, is an old idiom in Go. It’s clearly not seen as a best practice anymore since around 2018. Brad Fitzpatrick has stated that they stopped using it in the stdlib a long time ago. The conscensus on the popular go project standard project (not official) is to not use it either.
Well, that issue contains a lengthy discussion, was never resolved and the project layout still contains pkg, so it's just not true that there is consensus on that.
However, I've also observed the trend that it's used less than it was, and for Go projects which primarily contain packages intended to be used in other projects (like the stdlib), I agree that it does not add any value and just convolutes the import path. I would argue that things are different in more complex projects like authd, which are not primarily intended as a library but want to publish only a few packages for other projects that want to use authd, then it makes sense to put those packages in a directory which makes that purpose clear, and pkg/ still serves that purpose. I don't care enough to continue arguing for it though. Maybe we can find a different name than brokers/ or pkg/ to put those packages in?
|
|
||
| // Mode is the type to define an authd authentication Mode for brokers usage. | ||
| type Mode struct { | ||
| *authd.AuthMode |
There was a problem hiding this comment.
I don't like that this adds a dependency on protobuf and grpc to the broker (which doesn't actually use gRPC). I think we need to redefine the fields instead of embedding the protobuf message here.
I also think we can get rid of most of the boilerplate here by using JSON as an intermediate format when converting to/from protobuf and string map. I have some WIP to implement that but some more work is needed and I cannot in good conscience invest more time in it now (while I have other, scheduled tasks to work on). Do you think this could wait until next pulse?
There was a problem hiding this comment.
This was my concern too... But I wanted to get it done quickly and this allowed both API consistency and generated code.
So I had also the idea of just reusing it fully privately, but it would still likely lead to the same dependency problem.
Thus, I can definitely refactor this too (I had already thought about, then I decided to see how the shortcut was accepted, at least as an interim solution).
Not sure about passing from json though. It was also one of the reasons why I was using the authd protobufs types (as json conversion is ready and works great as we do for gdm), but I wanted to avoid doing native structs -> variant -> native map -> json -> native structs.
Even though I had it in my options.
One thing I can probably craft is instead a simple generator that reimplements the protobuf structures into something proto-less.
We've various of hardcoded strings around that are often repeated and makes the code hard to maintain and refactor in a safely way.
So replace them with const values and add some utility functions to generate previously hardcoded values.
I've put them into the internal package, but I feel that some of these definitions would be actually better if exported to be re-usable by
authd-oidc-brokerstoo, or at least to an intermediate library for writing authd brokers.Meanwhile, this makes authd code a bit less fragile and ideally easier to refactor.
UDENG-5162