Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

allow project configuration to skip packagers #843

Merged
merged 2 commits into from
Feb 1, 2019

Conversation

jmartin-tech
Copy link
Contributor

Description

Since windows builds now offer multiple packagers to execute, allow the project configuration to skip a packager if the maintainer does not want to build that package type.

In original APPX support PR #675, there was mention of discussion on how to supporting multiple package type for a single platform being addressed during rollout, since I don't see another existing option, this is my proposal on how to add user input to determine which package types are desired.

usage in a project DSL to omit APPX build:

package :appx do
  skip_packager true
end

Maintainers

Please ensure that you check for:

  • If this change impacts git cache validity, it bumps the git cache
    serial number
  • If this change impacts compatibility with omnibus-software, the
    corresponding change is reviewed and there is a release plan
  • If this change impacts compatibility with the omnibus cookbook, the
    corresponding change is reviewed and there is a release plan

@jmartin-tech jmartin-tech requested a review from a team as a code owner July 12, 2018 16:19
@jmartin-tech jmartin-tech force-pushed the skip_packager branch 3 times, most recently from d0767bf to 82d0436 Compare July 28, 2018 21:50
Copy link

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

I'm not qualified yet to comment on adding this feature, but I can suggest a correction to the error the input validation raises.

@jmartin-tech jmartin-tech force-pushed the skip_packager branch 2 times, most recently from 3f438d4 to 47d30ff Compare August 15, 2018 22:30
Copy link
Contributor

@tyler-ball tyler-ball left a comment

Choose a reason for hiding this comment

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

One comment but otherwise this LGTM

#
# @return [TrueClass, FalseClass]
# whether to skip this packager type or not
def skip_packager(val = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose we change the signature of this method to be def skip_packager(). IMO if the user is adding skip_packager to their packaging definition that is enough indication of what they want to do. The true or false does not seem necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea, however there are other options exposed on msi packagers that I based this on.

git grep "def.*\(val = false\)"
lib/omnibus/packagers/msi.rb:    def bundle_msi(val = false)
lib/omnibus/packagers/msi.rb:    def fast_msi(val = false)

I believe consistency is key to adoption of function. I offered another similar option in #842 and master currently offers the above two instances.

If a refactor is desired can I consolidate my 2 PRs and expand to refactor the other two instances which would make this a breaking api change, or this and #843 can land as is, then offer a refactor PR that includes the required version bump.

@jmartin-tech
Copy link
Contributor Author

This seems to have gotten a number of approvals, can someone take a look here and at #842 to see if we can get these merged?

@jmartin-tech
Copy link
Contributor Author

Checking in again to see about getting this and #842 into a release.

@marcparadise
Copy link
Member

Sorry this has taken so long, thanks for being patient with us. I'm working on getting this merged today.

@marcparadise
Copy link
Member

@jmartin-r7 would you be able to rebase this on master? If I do it locally, I'll have to make a new branch and replace the PR.

@jmartin-tech
Copy link
Contributor Author

I can rebase, there are no conflicts upstream as yet, will hold an hour or so encase would rather avoid need to get approvals again.

@marcparadise
Copy link
Member

@jmartin-r7 #875 will be merging shortly, then it will be safe to rebase. It fixes the CI errors you'll otherwise get after a rebase on current master.

Signed-off-by: Jeffrey Martin <Jeffrey_Martin@rapid7.com>
Signed-off-by: Jeffrey Martin <Jeffrey_Martin@rapid7.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants