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

(cleanup) pam: Move model code into specific package #173

Merged
merged 3 commits into from
Jan 15, 2024

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Jan 13, 2024

So we can have it into a non-main package, leaving into main only the code that has to be there for library or binary generations.

This is just a first iteration, let me know if we want to do further cleanups too.

@3v1n0 3v1n0 requested a review from a team as a code owner January 13, 2024 20:39
@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (edc6202) 83.31% compared to head (733a6a3) 83.42%.
Report is 2 commits behind head on main.

Files Patch % Lines
pam/pam.go 76.92% 1 Missing and 2 partials ⚠️
pam/adapter/authmodeselection.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   83.31%   83.42%   +0.10%     
==========================================
  Files          57       57              
  Lines        4562     4568       +6     
==========================================
+ Hits         3801     3811      +10     
+ Misses        590      587       -3     
+ Partials      171      170       -1     

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

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.

I like the idea, but the name adapter does not feel right/complete. Can we think about a better one?

I'll start thinking about one and will report back as soon as I have some ideas...

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Jan 15, 2024

@denisonbarbosa sure... That was the result of a Friday rambling shouting between me and @didrocks, so just an idea... Used also in #155.

Check the MM logs, but happy to hear if you have better suggestions!

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Some suggestions for you!

pam/adapter/model.go Outdated Show resolved Hide resolved
pam/adapter/return.go Outdated Show resolved Hide resolved
pam/adapter/return.go Outdated Show resolved Hide resolved
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Once you fix the generator for the tests and quality check to pass, you can merge :)

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Not fan about passing them as an unique struct to various functions TBH, especially in the new call where I would build the object from discrete args.

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Jan 15, 2024

Not fan about passing them as an unique struct to various functions TBH, especially in the new call where I would build the object from discrete args.

Well, thing is that in some places I'd need to pass isInteractive, isGdm or whatever... and feel that having just boolean parameters around is not something nice to maintain.

@didrocks
Copy link
Member

Yes, but a struct, with is something like an optional struct is worse. At worst, you can say this is something small than a client and embed this struct in the client, but here, initiliazing with a Parameter is really not a pattern which is idiomatic

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Jan 15, 2024

Yes, but a struct, with is something like an optional struct is worse. At worst, you can say this is something small than a client and embed this struct in the client, but here, initiliazing with a Parameter is really not a pattern which is idiomatic

I see, I'll revert this and handle it in the verbose way ;-).

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Jan 15, 2024

Ok, moved also pam_test inside internal package too, and reverted the Parameters change.

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

The tests are still failing. Rerequest once the tests pass.

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Jan 15, 2024

The tests are still failing. Rerequest once the tests pass.

Yeah, as always the last "cleanup" change broke things, I hope this last rebuild will be all green :)

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.

Yeah... the annoying part is that this package does so much... We might need to take another look into it later, but it's not worth blocking this because of it.

Nice work!

@3v1n0 3v1n0 requested a review from didrocks January 15, 2024 16:39
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Looking good now!

.github/workflows/qa.yaml Show resolved Hide resolved
So we can have it into a non-main package, leaving into main only the
code that has to be there for library or binary generations.
We're not using this anywhere else, so we can just hide it inside an
internal package.
@didrocks didrocks merged commit 20d204e into ubuntu:main Jan 15, 2024
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.

None yet

5 participants