-
Notifications
You must be signed in to change notification settings - Fork 14
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
Manifest #17
base: master
Are you sure you want to change the base?
Manifest #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR ! I'm not sure that I understand it completely (e.g. what does fish-pepper manifest else than just writing a log file ?)
I've included some comments, where I think that it doesn't fit the fish-pepper model (like introducing a dockerOrg
key in a fully user control config section where the user has full control of all keys).
However, I don't have any spare cycles to maintain that project properly anymore, so I suggest, that we make a branch here for fish-pepper 1.x which holds only bug fixes potentially needed for the current images, and you are free to continue on master for a release 2.x. I would hand over all admin rights for this repo so you are free to progress whithout me being a blocker.
How does this sound ?
} | ||
|
||
function findImagesInFile(arch) { | ||
var lines = fs.readFileSync('push-images.log', 'utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be better to pick the image names from the configuration than from a log file which adds some dependencies on your the user's usage flow ? Looking up the config like 'build' does shouldn't be that hard here, too.
manifest = manifest + 'docker manifest push ' + imageName.getImageNameWithVersion() + '\n\n'; | ||
writeStream = fs.createWriteStream('manifest.log', {flags: 'a'}); | ||
writeStream.write(manifest); | ||
writeStream.end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens with the manifest except than get written to a log file ?
var dockerOrg = undefined; | ||
for (type in types) { | ||
var param = paramValues[type]; | ||
var org = image.config.config[types[type]][param].dockerOrg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every parameter which has a meaning for fish-pepper should go below the fish-pepper configuration section. Everything else can be introduced and changed by the user freely. So that kind of dockerOrg should not into this section where the user has the full control of all parameter keys.
Add support for docker manifest