Migrate AWSAuth.jl into AWSCore.jl #88
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattBrzezinski
changed the title
|
bors try |
tryBuild succeeded |
omus
reviewed
Sep 23, 2019
Yeah I can definitely see that, personally I've stared using the I can spend some time and split this up, or make a follow up CR based off of this one for non-functional changes. Marking this as |
mattBrzezinski
changed the title
|
bors try |
mattBrzezinski
changed the title
tryBuild succeeded |
omus
reviewed
Sep 24, 2019
omus
requested changes
Oct 1, 2019
mattBrzezinski
commented
Oct 17, 2019
mattBrzezinski
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Build passes, the ECS job just needed a good kicking to pass.
I'll kick it off again to get the proper notification. |
|
bors try |
tryBuild succeeded |
|
bors r+ |
iamed2
approved these changes
Oct 24, 2019
bors bot
added a commit
that referenced
this pull request
Oct 24, 2019
88: Migrate AWSAuth.jl into AWSCore.jl r=iamed2 a=mattBrzezinski Overview --- This merge request is part of a larger overall refactor. Recently this merge request went through in [HTTP.jl](JuliaWeb/HTTP.jl#443), it introduced the ability to add custom layers into the request stack. With that merge request being introduced we can now move the `AWS4AuthLayer` away from the `HTTP.jl` project. We need to find a new home for `AWSAuthLayer`, at first glance it makes most sense to move this into the `AWSAuth.jl` package. However, looking at the state of the package, and open issues we can see why this might not be the best plan. `AWSAuth.jl` has an open [issue](JuliaCloud/AWSAuth.jl#11) to move `AWSCredentials` from `AWSCore.jl` into it. However, if we did this we would introduce a circular dependency between `AWSAuth.jl` and `AWSCore.jl`. To get around this circular dependency we would need to move [Services.jl](https://github.com/JuliaCloud/AWSCore.jl/blob/master/src/Services.jl) into its own package. I do not think this is the best decision as we are creating more packages and not solving the core issue with the state of these packages. `AWSAuth.jl` is a tiny package, which is just a duplicate of the `AWS4AuthLayer` in `HTTP.jl`. I propose that we move this entire functionality into `AWSCore.jl`, and archive `AWSAuth.jl`. The addition of this is not a heavy lift into `AWSCore.jl` and helps centralize these AWS packages. I think this is a good step forward and will help clean up the current state of the JuliaCloud packages. Afterwards we can start looking ahead and re-designing the architecture behind these packages. Next Steps --- If this merge request is approved, we can clean up `HTTP.jl` and remove the `AWS4AuthLayer` code from that package as it already exists in this one. We can then begin looking into refactoring this package and making it more modular. [AWSAuth.jl](https://github.com/JuliaCloud/AWSAuth.jl) can be archived as it no longer has a purpose. Links --- - [AWSAuth.jl - Issue - Current state of the package](JuliaCloud/AWSAuth.jl#11) - [HTTP.jl - CR - Add Custom Layers to request stack](JuliaWeb/HTTP.jl#443) - [HTTP.jl - Issue - Move AWS4AuthLayer](JuliaWeb/HTTP.jl#355) Issues deemed OOO that are logged: --- - #90 - #91 - #92 Co-authored-by: Matt Brzezinski <[email protected]>
Canceled |
|
bors try |
tryBuild succeeded |
|
bors try |
tryBuild succeeded |
Moved AWS4AuthLayer into this package from AWSAuth.jl. Originally wanted to move AWSCredentials into AWSAuth.jl however this would create a circular dependency. Instead we're moving AWS4AuthLayer into AWSCore.jl. This now allows us to clean up HTTP.jl and remove AWS functionality from it.
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Oct 28, 2019
88: Migrate AWSAuth.jl into AWSCore.jl r=mattBrzezinski a=mattBrzezinski Overview --- This merge request is part of a larger overall refactor. Recently this merge request went through in [HTTP.jl](JuliaWeb/HTTP.jl#443), it introduced the ability to add custom layers into the request stack. With that merge request being introduced we can now move the `AWS4AuthLayer` away from the `HTTP.jl` project. We need to find a new home for `AWSAuthLayer`, at first glance it makes most sense to move this into the `AWSAuth.jl` package. However, looking at the state of the package, and open issues we can see why this might not be the best plan. `AWSAuth.jl` has an open [issue](JuliaCloud/AWSAuth.jl#11) to move `AWSCredentials` from `AWSCore.jl` into it. However, if we did this we would introduce a circular dependency between `AWSAuth.jl` and `AWSCore.jl`. To get around this circular dependency we would need to move [Services.jl](https://github.com/JuliaCloud/AWSCore.jl/blob/master/src/Services.jl) into its own package. I do not think this is the best decision as we are creating more packages and not solving the core issue with the state of these packages. `AWSAuth.jl` is a tiny package, which is just a duplicate of the `AWS4AuthLayer` in `HTTP.jl`. I propose that we move this entire functionality into `AWSCore.jl`, and archive `AWSAuth.jl`. The addition of this is not a heavy lift into `AWSCore.jl` and helps centralize these AWS packages. I think this is a good step forward and will help clean up the current state of the JuliaCloud packages. Afterwards we can start looking ahead and re-designing the architecture behind these packages. Next Steps --- If this merge request is approved, we can clean up `HTTP.jl` and remove the `AWS4AuthLayer` code from that package as it already exists in this one. We can then begin looking into refactoring this package and making it more modular. [AWSAuth.jl](https://github.com/JuliaCloud/AWSAuth.jl) can be archived as it no longer has a purpose. Links --- - [AWSAuth.jl - Issue - Current state of the package](JuliaCloud/AWSAuth.jl#11) - [HTTP.jl - CR - Add Custom Layers to request stack](JuliaWeb/HTTP.jl#443) - [HTTP.jl - Issue - Move AWS4AuthLayer](JuliaWeb/HTTP.jl#355) Issues deemed OOO that are logged: --- - #90 - #91 - #92 Co-authored-by: Matt Brzezinski <[email protected]>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This merge request is part of a larger overall refactor. Recently this merge request went through in HTTP.jl, it introduced the ability to add custom layers into the request stack. With that merge request being introduced we can now move the
AWS4AuthLayeraway from theHTTP.jlproject.We need to find a new home for
AWSAuthLayer, at first glance it makes most sense to move this into theAWSAuth.jlpackage. However, looking at the state of the package, and open issues we can see why this might not be the best plan.AWSAuth.jlhas an open issue to moveAWSCredentialsfromAWSCore.jlinto it. However, if we did this we would introduce a circular dependency betweenAWSAuth.jlandAWSCore.jl. To get around this circular dependency we would need to move Services.jl into its own package. I do not think this is the best decision as we are creating more packages and not solving the core issue with the state of these packages.AWSAuth.jlis a tiny package, which is just a duplicate of theAWS4AuthLayerinHTTP.jl. I propose that we move this entire functionality intoAWSCore.jl, and archiveAWSAuth.jl. The addition of this is not a heavy lift intoAWSCore.jland helps centralize these AWS packages.I think this is a good step forward and will help clean up the current state of the JuliaCloud packages. Afterwards we can start looking ahead and re-designing the architecture behind these packages.
Next Steps
If this merge request is approved, we can clean up
HTTP.jland remove theAWS4AuthLayercode from that package as it already exists in this one. We can then begin looking into refactoring this package and making it more modular.AWSAuth.jl can be archived as it no longer has a purpose.
Links
Issues deemed OOO that are logged: