Common code reorganisation #197
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
lbajolet-hashicorp
force-pushed
the
auth_rework
branch
2 times, most recently
from
f251bb5 to
b223417
Compare
lbajolet-hashicorp
force-pushed
the
relax-JWT-Error
branch
from
2c2ce0e to
549c516
Compare
JenGoldstrich
approved these changes
Nov 10, 2023
|
This will be very nice to support |
The googlecompute builder used to contain a lot of code that was not only used by itself, but by the post-processors as well. This way of organising code is a bit counter-intuitive, and may be problematic when sharing code between components. To make it clear what is supposed to be available for every component of the plugin, we move everything to a common lib, so the builder, and post-processors, can all depend on it.
The import post-processor used to invoke some functions from the GCE API directly, with a series of options provided directly from its context. Since we want to move away from that model for authentication, in favour of something common to all components, this has to change, and so we move the few calls that were directly made into functions in the driver. Then we can use this driver from the post-processor in order to perform those actions. This also has the benefit of letting the driver perform the asynchronous actions like getting the state of the image, making the code more reliable as there's a timeout to the action instead of an endless loop until the image is ready.
The authentication code was duplicated for all components, making changes heavier than they need to be. To consolidate this, we move the authentication configuration to common, and the code that sets-up the credentials for GCE.
The CreateImage and CreateImageFromRaw functions fropm the GCE driver were very similar and contained a lot of duplicated code, for very little changes in between the two calls. Instead of having both with a lot of parameters, we simplify the function to accept an image specification, as exposed by the GCE client library, so the driver only takes care of performing the call, and manage its lifecycle through channels, then returning the image after it's created.
lbajolet-hashicorp
force-pushed
the
auth_rework
branch
from
4f72e1c to
37df63d
Compare
|
Not sure what the linter is complaining about, locally this doesn't fail so I'll override the test and merge this as-is, we'll fix the issue if it becomes persistent. |
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.
This PR reworks the code for all the components, now instead of having the common code as part of the builder, and have the post-processors depend on it, we moved all the common code to a
libpackage, which is then leveraged by components.This removes some duplication in the authentication logic, and consolidates it in a single package, along with the driver, which gets more capabilities for the post-processors to use.
This however means (and this was already the case in the related PR) that the
googlecompute-exportcomponent now needs to expose its own authentication methods, as it cannot rely on the artifact for that.Leaving authentication information in the artifact may be a security issue later, so we opted to remove that.
Note: based on top of #186