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

Feature/on error http response #174

Merged
merged 6 commits into from
May 12, 2021
Merged

Feature/on error http response #174

merged 6 commits into from
May 12, 2021

Conversation

joe94
Copy link
Member

@joe94 joe94 commented May 11, 2021

No description provided.

@joe94 joe94 linked an issue May 11, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #174 (3121967) into main (4761625) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 3121967 differs from pull request most recent head d285fc8. Consider uploading reports for the commit d285fc8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
+ Coverage   63.20%   63.24%   +0.04%     
==========================================
  Files          16       16              
  Lines         875      876       +1     
==========================================
+ Hits          553      554       +1     
  Misses        306      306              
  Partials       16       16              
Impacted Files Coverage Δ
chrysom/client.go 90.84% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4761625...d285fc8. Read the comment docs.

if logger == nil {
logger = bascule.GetDefaultLoggerFunc
if getLogger == nil {
getLogger = func(ctx context.Context) log.Logger {
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like it might be useful to bring back the GetDefaultLoggerFunc function (one returning a log.Logger).

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're moving away from log.Logger, I'm not sure we should put in the work?

},
},
fx.Annotated{
Name: "primary_bearer_validator_type",
Target: func() bascule.Validator {
return bascule.CreateValidTypeCheck([]string{"jwt"})
return bchecks.ValidType([]string{"jwt"})
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought basculechecks implemented capability checks so I found it a bit surprising these validators got moved there.

Copy link
Member Author

Choose a reason for hiding this comment

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

But in any case, we'll have both of these packages imported together until we fully transition to touchstone.

Copy link
Contributor

@kristinapathak kristinapathak May 11, 2021

Choose a reason for hiding this comment

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

basculechecks is for all checks aka bascule.Validators, which is why I moved those there. I didn't think about the double import... 😬 luckily long term this will go away.

@joe94 joe94 requested a review from kristinapathak May 11, 2021 00:26
Copy link
Contributor

@kristinapathak kristinapathak left a comment

Choose a reason for hiding this comment

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

Looks good! Just had one question in the constructor setup.

Group: primaryBasculeCOptionsName,
Target: func(in primaryBasculeOnHTTPErrorResponseIn) basculehttp.COption {
if in.OnErrorHTTPResponse == nil {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this add nil to the list? I'm not sure that bascule is equipped to handle that?

A solution might be to use the arrange.IfSet() function, but I'm not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, nil will be added to the list but it'll be filtered out before using the options here

var filteredOptions []basculehttp.COption

if logger == nil {
logger = bascule.GetDefaultLoggerFunc
if getLogger == nil {
getLogger = func(ctx context.Context) log.Logger {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're moving away from log.Logger, I'm not sure we should put in the work?

@sonarqubecloud
Copy link

@joe94 joe94 merged commit 2d99e76 into main May 12, 2021
@joe94 joe94 deleted the feature/onErrorHTTPResponse branch May 12, 2021 19:36
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.

Upgrade bascule version for the auth package
2 participants