-
Notifications
You must be signed in to change notification settings - Fork 212
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
Generate a mixin feed based on a directory #279
Conversation
This template shows how to use the mustache templating and is a working template. From there the user can customize it if they want to use more atom features, but this is all porter needs to do its magic.
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.
Only a couple of (non-blocking) questions; LGTM!
pkg/mixin/feed/generate.go
Outdated
} | ||
|
||
if _, err := cxt.FileSystem.Stat(o.SearchDirectory); err != nil { | ||
return errors.Wrapf(err, "invalid --directory %s", o.SearchDirectory) |
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.
Q: should we print invalid --dir
or invalid -d
to represent the flag strings supported? Or perhaps just invalid directory
for a more general error msg?
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.
oops, yes thanks it should be --dir
pkg/mixin/feed/generate.go
Outdated
return errors.Wrapf(err, "error reading template file at %s", opts.TemplateFile) | ||
} | ||
|
||
mixinRegex := regexp.MustCompile(`(.*/)?(.+)/([a-z]+)-([a-z0-9]+)-([a-z0-9]+)(\.exe)?`) |
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.
Do we want to restrict some of these regex fields to only the currently supported values? (For instance, OS can be only one of {linux, windows, darwin}
, arch only amd64
, etc.)
I can also understand wanting to keep it unrestricted with a vision towards supporting other variants...
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.
I originally didn't want to be in the business of keeping up to date with our build matrix but honestly, it is not going to change often. So we just need to be aware that this is here. I'll update it and leave a note in the makefile's build matrix that this should be updated too when it's changed.
* Limit the regex for matching mixin files to the currently supported os/arch in our build matrix * Fix the flag in our error message to be --dir
This adds two commands to porter as a build up to supporting mixin feeds. All alone they aren't really enough to add new docs or anything yet though.
Note: This doesn't update an existing atom.xml file yet. I plan on doing that in a follow-up PR.