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

Add package for mixins magefile targets #1852

Merged
merged 6 commits into from
Jan 12, 2022

Conversation

carolynvs
Copy link
Member

@carolynvs carolynvs commented Jan 7, 2022

What does this change

The mage/mixins package contains common magefile targets that all mixins
use: build, test, publish, etc. This minimizes the amount of custom
logic in our magefiles and should make it easier to manage the magefile
in all the mixins going forward.

What issue does it fix

Deals with some longstanding drift between all the mixins build script logic. This should get them all on the same page.

Notes for the reviewer

I am testing this out with the docker-compose mixin first in getporter/docker-compose-mixin#23. Please review these two PRs together and let me know if this looks like the right change to make to EVERY mixin repo. Once this is merged I'll make the same change everywhere, so I'd like to make sure this first PR has everything we want in it so I only have to update repos once. 😀

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

The mage/mixins package contains common magefile targets that all mixins
use: build, test, publish, etc. This minimizes the amount of custom
logic in our magefiles and should make it easier to manage the magefile
in all the mixins going forward.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs carolynvs marked this pull request as ready for review January 7, 2022 21:12
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

LGTM!

Use the system porter that was installed by the EnsurePorter target.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs carolynvs force-pushed the common-magefile-functions branch from 88e8c73 to b48237d Compare January 10, 2022 20:03
carolynvs added a commit to carolynvs/docker-compose-mixin that referenced this pull request Jan 10, 2022
This uses newer logic from the Porter PR
getporter/porter#1852:

* Do not use --author with git commit because git still wants an
identity. Set the config file values directly on the command, e.g. -c
user.name=x -c user.email=y to temporarily set those values.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Allow someone to just publish binaries or also publish a mixin feed.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs
Copy link
Member Author

@vdice I tested out this branch on skeletor and the docker-compose mixin and made a few more changes. Would you please take a look at the new commits and let me know if they look ok?

I plan to update the remaining mixins after we merge this and tag a new prerelease.

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Non-blocking Q/comment; LGTM still stands.


repo := os.Getenv("PORTER_RELEASE_REPOSITORY")
repo := os.Getenv(ReleaseRepository)
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we want to specifically error out if repo is empty (similarly for publishPackageFeed below and its remote value)? Or, will it be obvious to the user that an empty value (via an unset env var) is to blame for this target not working?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be obvious, because it defaults to publishing to Porter's repos if nothing is overridden with env vars, just a wee bit further down in this function. So they will see errors with those repo names saying that they don't have access.

I have a TestPublish target that people can use as well, it assumes that the repo is named a certain way.

$ mage TestPublish carolynvs
# sets the repo to github.com/carolynvs/MIXINNAME-mixin
# sets the package remote to github.com/carolynvs/packages.git

Though people are always free to override those env vars and just call publish directly to test if they use different repo names.

Print out where we are publishing and explain what to do if you use
different repository names.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
mixinRepo := fmt.Sprintf("github.com/%s/%s-mixin", username, m.MixinName)
pkgRepo := fmt.Sprintf("https://github.com/%s/packages.git", username)
fmt.Printf("Publishing a release to %s and committing a mixin feed to %s\n", mixinRepo, pkgRepo)
fmt.Printf("If you use different repository names, set %s and %s then call mage Publish instead.\n", releases.ReleaseRepository, releases.PackagesRemote)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@carolynvs carolynvs merged commit d7d2341 into getporter:release/v1 Jan 12, 2022
@carolynvs carolynvs deleted the common-magefile-functions branch January 12, 2022 17:13
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.

2 participants