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

pam: Add GDM JSON protocol definition #121

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Nov 30, 2023

We use JSON to communicate with GDM since its UI, GNOME Shell is in JavaScript and so can handle those messages natively pretty well.

Here we define the primitives of this protocol and their conversion back and forth from go structures and types, ensuring the expectations are met.

The protocol code generation is mostly done via protobuf so that we can avoid manual definition of many structures, but is meant for being used from the JSON side too thus using some naming conventions to please both cases.

UDENG-1466

@3v1n0 3v1n0 requested a review from a team as a code owner November 30, 2023 08:24
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Hey, @3v1n0! Some minor nitpicks here and there.

But there's a major but: It's very hard to review and approve a protocol definition without seeing it being used and hooked up, so these changes should come together with the actual GDM module implementation rather than in a standalone PR.

pam/gdm/gdm.proto Outdated Show resolved Hide resolved
pam/gdm/protocol_test.go Outdated Show resolved Hide resolved
pam/gdm/protocol_test.go Outdated Show resolved Hide resolved
pam/gdm/protocol_test.go Outdated Show resolved Hide resolved
@3v1n0 3v1n0 force-pushed the gdm-json-protocol-protobuf branch 3 times, most recently from 36ff68c to c498594 Compare November 30, 2023 15:14
@denisonbarbosa
Copy link
Member

I'll move this to draft, as we discussed during the HO.

@denisonbarbosa denisonbarbosa marked this pull request as draft December 1, 2023 14:37
@3v1n0 3v1n0 force-pushed the gdm-json-protocol-protobuf branch from c498594 to 6238320 Compare December 2, 2023 04:02
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (06c32ac) 88.62% compared to head (9f4f56d) 88.82%.

Files Patch % Lines
pam/gdm/protocol.go 95.37% 4 Missing and 1 partial ⚠️
pam/gdm/extension.go 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   88.62%   88.82%   +0.20%     
==========================================
  Files          34       35       +1     
  Lines        2532     2640     +108     
==========================================
+ Hits         2244     2345     +101     
- Misses        221      227       +6     
- Partials       67       68       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@3v1n0 3v1n0 force-pushed the gdm-json-protocol-protobuf branch 2 times, most recently from 53cf468 to 4e9ba77 Compare December 11, 2023 21:17
@3v1n0 3v1n0 marked this pull request as ready for review December 11, 2023 21:17
@3v1n0 3v1n0 requested a review from denisonbarbosa December 11, 2023 21:18
@3v1n0 3v1n0 force-pushed the gdm-json-protocol-protobuf branch from 4e9ba77 to 8638e82 Compare December 11, 2023 21:25
pam/gdm/protocol_test.go Outdated Show resolved Hide resolved
pam/gdm/protocol_test.go Outdated Show resolved Hide resolved
pam/gdm/protocol.go Outdated Show resolved Hide resolved
pam/gdm/protocol.go Outdated Show resolved Hide resolved
pam/gdm/protocol_test.go Outdated Show resolved Hide resolved
GabrielNagy added a commit that referenced this pull request Dec 13, 2023
…#142)

Cleanup the way we generate the targets moving some stuff to scripts so
that they are more maintainable and and reusable (I'd need the proto
generation for #121 for example).

Plus address an issue we have with current module path loader when using
relative paths.
@3v1n0 3v1n0 force-pushed the gdm-json-protocol-protobuf branch 8 times, most recently from 4ee594a to 0b84f6b Compare December 14, 2023 05:11
Copy link
Contributor

@GabrielNagy GabrielNagy left a comment

Choose a reason for hiding this comment

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

Great work! This is definitely cleaner than the previous approach - I had some minor comments

pam/gdm/protocol.go Outdated Show resolved Hide resolved
pam/gdm/protocol.go Outdated Show resolved Hide resolved
pam/gdm/extension.go Outdated Show resolved Hide resolved
@3v1n0 3v1n0 requested a review from GabrielNagy December 14, 2023 20:55
Copy link
Contributor

@GabrielNagy GabrielNagy left a comment

Choose a reason for hiding this comment

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

Couple more things I spotted

pam/gdm/debug.go Outdated Show resolved Hide resolved
pam/generate_debug.go Outdated Show resolved Hide resolved
@3v1n0 3v1n0 requested a review from GabrielNagy December 15, 2023 11:24
Copy link
Contributor

@GabrielNagy GabrielNagy left a comment

Choose a reason for hiding this comment

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

Nice work, I'll let you squash before merging :)

We use JSON to communicate with GDM since its UI, GNOME Shell is in
JavaScript and so can handle those messages natively pretty well.

Here we define the primitives of this protocol and their conversion back
and forth from go structures and types, ensuring the expectations are
met.

The protocol code generation is mostly done via protobuf so that we can
avoid manual definition of many structures, but is meant for being
used from the JSON side too thus using some naming conventions to please
both cases.
We may want to enable some checks only during checks or debug builds to
be used in integration tests, so let's abstract things a bit more by
exposing some features at runtime only.

This allows to generate normal builds without debug checks but at the
same time, to avoid having to define special build tags when testing,
leaving the room for using special tags for building different binaries
for integration tests or local debugging.
When using this tag on `go build` or `go generate` we:
 - Add more debug symbols to built libraries
 - Enable pedantic checks on data structures used by GDM proto

To achieve this, add a further generate_debug.go file that is only
used when the `pam_debug` tag is used, this now also builds the gdm
code with the `pam_gdm_debug` build tag that implies enabling by
default some checks that are not enabled by default.

Update CI job to ensure this works.
@3v1n0 3v1n0 force-pushed the gdm-json-protocol-protobuf branch from b328b7b to 9f4f56d Compare December 15, 2023 13:36
@3v1n0
Copy link
Collaborator Author

3v1n0 commented Dec 15, 2023

Thanks, all squashed now :)

@GabrielNagy GabrielNagy merged commit ba9f0e2 into ubuntu:main Dec 15, 2023
4 checks passed
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.

4 participants