pkg/asset/installconfig/aws/metadata: Store AWS metadata #2512
Conversation
"aws.go" doesn't add any information that is not covered in the package name. Putting Platform in its own platform.go file will make it easier to find for folks tab-completing into the directory in search of Platform.
"aws.go" doesn't add any information that is not covered in the package name. Putting session handling in its own session.go file will make it easier to find for folks tab-completing into the directory in search of GetSession and friends.
Consolidating finalizing logic which is shared between Generate and Load. This will make it easier to add additional shared logic going forward, because you'll only have to add it to one place. Also inline setDefaults and convert instead of using separate methods for those one-liners.
openshift-ci-robot
added
the
size/L
openshift-ci-robot
added
the
approved
| return errors.Wrap(err, "invalid install config") | ||
| } | ||
|
|
||
| data, err := yaml.Marshal(a.Config) |
There was a problem hiding this comment.
would like to keep this part in the asset itself.
and finish could take *InstallConfig for better api.
There was a problem hiding this comment.
would like to keep this part in the asset itself.
We currently marshal in Generate and in Load (presumably to capture any default injection so it doesn't need to get re-injected next time, it was added here). So I think it makes sense to have it in the shared finish helper, but can push it back up into the doubled Generate / Load logic if you prefer. Thoughts?
and
finishcould take*InstallConfigfor better api.
You want func finish(a *InstallConfig, filename string) error instead of func (a *InstallConfig) finish(filename string) error? I don't see a benefit to a function over my current method, but I can reroll to use a function if you like. Just let me know that I'm reading your comment correctly.
There was a problem hiding this comment.
don't care enough. It's fine as is.
This allows us to hold the AWS session (and in the future, other metadata) on the InstallConfig object, where we can load it once in InstallConfig.Generate or InstallConfig.Load, instead of having to call GetSession or other loaders in all consumers. At the moment, I'm just stubbing in the framework; I'll port our other GetSession consumers in future commits. I'm using private properties with getters so we don't have to pay the price of initializing properties we don't need to use. Consumers can call the getter, which will pull fresh data into the caching property the first time, and return the previously cached data on subsequent calls.
This gives us caching so we no longer pull zones multiple times. Previously, we'd pull zones for each pool that did not configure zones. It also moves us from a per-zone-request GetSession calls to the new Metadata.Session() cache, which saves repetition there as well.
wking
force-pushed
the
aws-metadata-caching
branch
from
d399c01 to
1b4c1bf
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
5 participants
This allows us to hold the AWS session (and in the future, other metadata) on the
InstallConfigobject, where we can load it once inInstallConfig.GenerateorInstallConfig.Load, instead of having to callGetSessionor other loaders in all consumers.I'm using private properties with getters so we don't have to pay the price of initializing properties we don't need to use. Consumers can call the getter, which will pull fresh data into the caching property the first time, and return the previously cached data on subsequent calls.
The PR is a series of commits with small pivots to set the stage for the metadata helper, add the helper, and then move availability zone calculation into the helper. See the individal commit messages for more details on each pivot.