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 (re)using string values for layout types as is, since they are prone to errors and they're hard to maintain.
Avoid (re)using string values for entries types as is, since they are prone to errors and they're hard to maintain.
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.
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