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

twoliter: refactor lock & project interfaces #363

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

cbgbt
Copy link
Contributor

@cbgbt cbgbt commented Sep 7, 2024

Description of changes:
While working on #361 I started to consider how to better close-off some of these verification abstractions. Separately, I had an idea on how to make it so that ImageResolver could be less aware of how overrides are stored.

This PR contains two minor refactors to existing APIs to try and more-tightly control usage of those APIs via privacy and type constraints.

    twoliter: refactor lock to be a private project submodule
    
    The lock abstractions force the project interface to perform the
    appropriate validations when resolving and using dependencies.
    This change moves the `lock` module under `project` so that we can
    more-tightly control access to that functionality.

    twoliter: force vendor lookups to deal with overrides
    
    This funnels vendor lookups into receiving an enum, which forces
    callers handling an image's vendor to explicitly handle overrides.

    twoliter: consistently provide image override view
    
    Twoliter had a bug in which overridden SDK images were resolved, but then
    the wrong SDK was being passed into builds.
    
    This change:
    * Uses the the sealed trait/typestate pattern to modify the Project with
      interfaces to fetch images based on the lock level.
    * Provides clarified public interfaces for getting images from pre/post
      lock resolution.
    * Forces public and private callers to deal with possibly-overridden
      images.

Testing done:

  • Unit tests pass
  • built bottlerocket-core-kit
  • built bottlerocket variants

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

This funnels vendor lookups into receiving an enum, which forces
callers handling an image's vendor to explicitly handle overrides.
The lock abstractions force the project interface to perform the
appropriate validations when resolving and using dependencies.
This change moves the `lock` module under `project` so that we can
more-tightly control access to that functionality.
Copy link
Contributor

@jmt-lab jmt-lab left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 298 to 302
pub(crate) fn sdk_image(&self) -> ProjectImage {
let Locked(lock) = &self.lock;
self.as_project_image(&lock.sdk)
.expect("Could not find kit vendor despite lock resolution succeeding?")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right error message?

Suggested change
pub(crate) fn sdk_image(&self) -> ProjectImage {
let Locked(lock) = &self.lock;
self.as_project_image(&lock.sdk)
.expect("Could not find kit vendor despite lock resolution succeeding?")
}
pub(crate) fn sdk_image(&self) -> ProjectImage {
let Locked(lock) = &self.lock;
self.as_project_image(&lock.sdk)
.expect("Could not find SDK vendor despite lock resolution succeeding?")
}

@cbgbt
Copy link
Contributor Author

cbgbt commented Sep 11, 2024

^ force push for feedback from @bcressey

Twoliter had a bug in which overridden SDK images were resolved, but then
the wrong SDK was being passed into builds.

This change:
* Uses the the sealed trait/typestate pattern to modify the Project with
  interfaces to fetch images based on the lock level.
* Provides clarified public interfaces for getting images from pre/post
  lock resolution.
* Forces public and private callers to deal with possibly-overridden
  images.
@cbgbt cbgbt merged commit bf28d26 into bottlerocket-os:develop Sep 11, 2024
1 check passed
@cbgbt cbgbt deleted the protect-lock branch September 11, 2024 00:42
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.

3 participants