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

feat(publish): set aptly Origin + Label #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

caugner
Copy link

@caugner caugner commented Feb 3, 2022

Fixes #58.

@mattermod
Copy link
Contributor

Hello @caugner,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @aspleenic

@caugner
Copy link
Author

caugner commented Apr 11, 2022

@amyblias Who might be able to review this PR?

@mgdelacroix
Copy link
Member

@caugner thanks for your contribution and apologies for the delay. I can take a look

@mgdelacroix mgdelacroix requested review from mgdelacroix and a user April 11, 2022 11:18
@mgdelacroix mgdelacroix added the 2: Dev Review Requires review by a core committer label Apr 11, 2022
@ghost ghost requested review from phoinixgrr and spirosoik April 11, 2022 11:19
Copy link
Member

@spirosoik spirosoik left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -4,6 +4,8 @@ set -xeuo pipefail
release=$1
opt=${2:-false}
nightly=false
origin="Mattermost"
Copy link

@ghost ghost Apr 11, 2022

Choose a reason for hiding this comment

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

Is it possible to fetch origin and label from arguments and make them mandatory?

So that we can distinguish packages by origin. As Mattermost we may want to set origin from CI/CD pipeline to determine officially generated packages. And for anyone who builds and publish to their own repository, they may want to use different origin and label value.

Copy link
Member

Choose a reason for hiding this comment

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

pretty good point yes

Copy link
Author

Choose a reason for hiding this comment

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

Is it possible to fetch origin and label from arguments and make them mandatory?

Currently, the script takes two parameters:

  1. release
  2. (optional) opt

Should origin and label be mandatory parameters 2 and 3 as follows:

  1. release
  2. origin
  3. label
  4. (optional) opt

Or might you prefer to facilitate environment variables APT_RELEASE, APT_ORIGIN and APT_LABEL for this? The script could still fail if those are not set.

Copy link

@ghost ghost Apr 13, 2022

Choose a reason for hiding this comment

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

Thank you very much, using enviroment veriables is great idea.

And since we will have multiple parameters, using environment variables will increase readability and flexibility.

./sync-and-publish bionic "Mattermost" "Mattermost Omnibus" --nightly

-vs-

APT_RELEASE="bionic" APT_ORIGIN="Mattermost" APT_LABEL="Mattermost Omnibus" ./sync-and-publish --nightly

It will be nice to use Environment variables and both of them are okay.

@phoinixgrr phoinixgrr removed their request for review August 18, 2022 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer Lifecycle/1:stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to update Mattermost-Omnibus with unattended-upgrade?
4 participants