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

Dependency mappings #4

Merged
merged 3 commits into from
Jul 17, 2020
Merged

Dependency mappings #4

merged 3 commits into from
Jul 17, 2020

Conversation

ekcasey
Copy link
Member

@ekcasey ekcasey commented Jul 15, 2020

Resolves #3

This allows users in an airgapped environment or an environment with restrictive firewall rules to map dependencies to new URIs that are accessible from the build environment

Signed-off-by: Emily Casey <[email protected]>
@ekcasey ekcasey requested a review from nebhale July 15, 2020 22:40
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 15, 2020

CLA Check
The committers are authorized under a signed CLA.

Signed-off-by: Emily Casey <[email protected]>
effect/mocks/executor.go Outdated Show resolved Hide resolved
)

// MappingsFile defines dependency mappings for a set of buildpacks
type MappingsFile struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why not hide this implementation details?

}

// ReadMappingsFile read MappingsFile from path
func ReadMappingsFile(path string) (MappingsFile, error) {
Copy link
Member

Choose a reason for hiding this comment

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

By returning ([]BuildpackMappings, error) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that originally but I thought somebody could realistically want to use this library to write the file in the correct format

Copy link
Member Author

Choose a reason for hiding this comment

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

happy to change it back if you think that is unrealistic

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wouldn't optimize for that right now. It's a good thought, but I'm pretty realistic that we're the only ones that are going to use this.

dependency_mapping.go Outdated Show resolved Hide resolved
dependency_cache.go Outdated Show resolved Hide resolved
dependency_mapping.go Outdated Show resolved Hide resolved
dependency_cache.go Outdated Show resolved Hide resolved
var mappings []DependencyMapping
for _, bpm := range mappingsFile.BuildpackMappings {
if bpm.BuildpackID == context.Buildpack.Info.ID {
mappings = bpm.Mappings
Copy link
Member

Choose a reason for hiding this comment

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

There should also be a log message (not sure of how prominent we want it to be) that mentions that mappings are going to be used for this buildpack.

Copy link
Member Author

Choose a reason for hiding this comment

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

Omitting this for now because the Logger is not injected into the constructor

@ekcasey ekcasey requested a review from nebhale July 16, 2020 16:10
* Better error messages
* Handling missing mappings file
* Don't export entire file schema

Signed-off-by: Emily Casey <[email protected]>
@ekcasey ekcasey merged commit d455ee4 into main Jul 17, 2020
@nebhale nebhale added semver:minor A change requiring a minor version bump type:enhancement A general enhancement labels Sep 23, 2020
@nebhale nebhale deleted the dependency-mappings branch October 10, 2020 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support a platform-dependent dependency mapping
2 participants