Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Feature/selectable middleware #295

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Conversation

teemukataja
Copy link
Contributor

Background

We have use cases at CSC where we are using sda-download in three different contexts, and each of them have a different authentication middleware. We have been maintaining three separate implementations, and would now like to streamline them into a single codebase. These changes here would lay the foundation for the capabilities we need (changing middleware, more versatile cache), and then we would implement our needs in our private repository.

Changes

  • new config app.middleware, with a default value default
  • TokenMiddleware is now the default middleware, custom middlewares can be built in the middleware package, and be selected for runtime using the config (added a readme to the middleware directory for instructions)
  • session cache now works with the whole cache struct instead of only the datasets slice, so that it can be used to store more items
  • no breaking changes, only refactoring to allow us to use the same codebase for more use cases! 😄

@teemukataja teemukataja added the enhancement New feature or request label Jul 24, 2023
@teemukataja teemukataja requested a review from a team July 24, 2023 07:58
@teemukataja teemukataja self-assigned this Jul 24, 2023
@teemukataja teemukataja marked this pull request as ready for review July 24, 2023 08:04
@blankdots blankdots requested a review from a team July 24, 2023 08:15
Copy link
Contributor

@pontus pontus left a comment

Choose a reason for hiding this comment

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

Looks OK.

Could possibly have a short explainer in middleware.go to clarify the cache flow (at least I got tripped up a few times thinking about it), but I'm fine with merging as is.

@teemukataja
Copy link
Contributor Author

Looks OK.

Could possibly have a short explainer in middleware.go to clarify the cache flow (at least I got tripped up a few times thinking about it), but I'm fine with merging as is.

I agree, I'll see if I can refactor it a little bit after this PR to simplify it

@codecov-commenter
Copy link

Codecov Report

Merging #295 (6f21386) into main (c14131c) will decrease coverage by 0.11%.
Report is 16 commits behind head on main.
The diff coverage is 88.23%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
- Coverage   67.45%   67.34%   -0.11%     
==========================================
  Files          10       10              
  Lines        1598     1611      +13     
==========================================
+ Hits         1078     1085       +7     
- Misses        484      489       +5     
- Partials       36       37       +1     
Flag Coverage Δ
unittests 67.34% <88.23%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
internal/config/config.go 58.36% <14.28%> (-1.37%) ⬇️
api/api.go 93.18% <100.00%> (+0.32%) ⬆️
api/middleware/middleware.go 100.00% <100.00%> (ø)
api/s3/s3.go 62.22% <100.00%> (+0.64%) ⬆️
api/sda/sda.go 85.78% <100.00%> (ø)
internal/session/session.go 91.22% <100.00%> (-0.71%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@teemukataja teemukataja merged commit 7babab0 into main Aug 29, 2023
5 checks passed
@teemukataja teemukataja deleted the feature/selectable-middleware branch August 29, 2023 06:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants