Skip to content

ci: add Chocolatey support #8921

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 16, 2023
Merged

ci: add Chocolatey support #8921

merged 8 commits into from
Aug 16, 2023

Conversation

phorcys420
Copy link
Member

@phorcys420 phorcys420 commented Aug 5, 2023 •
edited
Loading

Hello all, I'm finally contributing Chocolatey building (#3714) to the project long after I said I would do it.

This is a draft because there are some questions that need to be answered by the team:

  1. should copyright be uncommented ? (L26 of coder.nuspec) (I think it should be removed since this is the community edition right ?
  2. should requireLicenseAcceptance be uncommented ? (L28 of coder.nuspec)
  3. someone from the coder team (cc: @bpmct) needs to create an account @ https://community.chocolatey.org and define the CHOCO_API_KEY GH secret with the API key.

Other than that, I have tested all the PowerShell code in PS v7 and it works like intended (except for choco push).
I could not get the GH action to run in my repository, so that's the only thing I see that might cause issues, because the action itself was not tested. @matifali recommended that I don't bother trying to get the action running in my own repo, so here I am!

Now that I'm looking at this, there is another question I would like asking, does the winget job actually need to checkout and fetch tags ? from what I see it doesn't actually need the repo to be cloned at all.

@cdr-bot cdr-bot bot added the community Pull Requests and issues created by the community. label Aug 5, 2023
@phorcys420 phorcys420 mentioned this pull request Aug 5, 2023
Closed
deansheather
deansheather reviewed Aug 6, 2023
View reviewed changes
@deansheather
Copy link
Member

Now that I'm looking at this, there is another question I would like asking, does the winget job actually need to checkout and fetch tags ? from what I see it doesn't actually need the repo to be cloned at all.

You're probably right.

@deansheather
Copy link
Member

  1. should copyright be uncommented ? (L26 of coder.nuspec) (I think it should be removed since this is the community edition right ?

The binaries we ship include enterprise code. We don't ship AGPL binaries and don't plan to. The enterprise binaries will not use enterprise features without a valid license. This should probably be uncommented

  1. should requireLicenseAcceptance be uncommented ? (L28 of coder.nuspec)

Yes, probably. With both the enterprise license and AGPL license shown if possible

@phorcys420
Copy link
Member Author

With both the enterprise license and AGPL license shown if possible

It seems that the nuspec only accepts one licenseUrl
I think what we can do though is expose a LICENSE.mixed at the root of the repo or on coder.com

Also, should this use slim binaries instead ?

@phorcys420 phorcys420 requested a review from deansheather August 6, 2023 06:43
@phorcys420 phorcys420 changed the title ci: add Chocolatey manifest ci: add Chocolatey support Aug 6, 2023
@deansheather
Copy link
Member

It seems that the nuspec only accepts one licenseUrl
I think what we can do though is expose a LICENSE.mixed at the root of the repo or on coder.com

I guess? @bpmct

@deansheather
Copy link
Member

Also, should this use slim binaries instead ?

No this should be fat binaries from GitHub releases. We don't ship slim binaries except for use in agents and our vscode extension

@phorcys420
Copy link
Member Author

I have applied your modifications and I am waiting for @bpmct's input on the license thing.

Should I squash the license change into the "ci: add Chocolatey manifest" commit or simply create another commit ?

deansheather
deansheather reviewed Aug 7, 2023
View reviewed changes
Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

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

LGTM, we just need Ben or someone else to add API keys and comment on the license thing

@bpmct
Copy link
Member

bpmct commented Aug 7, 2023 •
edited
Loading

It seems that the nuspec only accepts one licenseUrl
I think what we can do though is expose a LICENSE.mixed at the root of the repo or on coder.com

I guess? @bpmct

I'm in favor of a LICENSE.mixed. We're submitting to other package manager things such as AWS Marketplace and they also only support a single license URL.

Edit: this does not seem that common. Let's link to the enterprise license instead since that is what we distribute.

@bpmct
Copy link
Member

bpmct commented Aug 7, 2023

We set CHOCO_API_KEY as a GitHub actions secret 👍🏼

@phorcys420
Copy link
Member Author

phorcys420 commented Aug 8, 2023 •
edited
Loading

Edit: this does not seem that common.

Yes but I think it also makes sense given that multi-licensing is not so common, and maybe they don't use the name "LICENSE.mixed", that was simply a suggestion.

Let's link to the enterprise license instead since that is what we distribute.
Well, we do distribute both licences in the zips and the installer even creates its own mixed license.

I think it would make sense to follow this behavior since we're already doing it with the rest.
It will also be an issue with scoop since it also accepts a single license and it wouldn't make sense that all of the packages that can only accept a single license combine the license themselves like the installer does, it would be redundant.

@matifali
Copy link
Member

matifali commented Aug 9, 2023

@phorcys420, is it ready for merging?

@phorcys420
Copy link
Member Author

@matifali I was still waiting for Ben because I think we should really use the dual license.
But I just realized that I can simply set the enterprise license for now and change it later.

I will change the license right now.

@phorcys420
Copy link
Member Author

sorry, had to force-push because I am not used to git yet and merged the main branch on accident.

@phorcys420 phorcys420 marked this pull request as ready for review August 9, 2023 13:32
@phorcys420 @matifali
ci: update chocolatey license to ToS
7de6b6e 
Co-authored-by: Muhammad Atif Ali <matifali@live.com>
@matifali matifali requested a review from deansheather August 15, 2023 16:02
@matifali matifali requested a review from bpmct August 15, 2023 16:02
deansheather
deansheather approved these changes Aug 15, 2023
View reviewed changes
phorcys420 and others added 2 commits August 16, 2023 11:31
@phorcys420 @deansheather
ci: remove semicolons from chocolatey install script
b69f74e 
Co-authored-by: Dean Sheather <dean@deansheather.com>
@phorcys420 @deansheather
ci: remove useless comment from chocolatey workflow
eda20c1 
Co-authored-by: Dean Sheather <dean@deansheather.com>
@phorcys420 @matifali
fix(ci): fix typo in choco apikey command
d8e18a5 
Co-authored-by: Muhammad Atif Ali <matifali@live.com>
@matifali
Copy link
Member

Let's merge it. :)

@matifali matifali merged commit f3c7076 into coder:main Aug 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2023
@phorcys420 phorcys420 deleted the chocolatey branch August 16, 2023 14:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@matifali matifali matifali approved these changes

@deansheather deansheather deansheather approved these changes

@bpmct bpmct Awaiting requested review from bpmct

Assignees

@phorcys420 phorcys420

Labels
community Pull Requests and issues created by the community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@phorcys420 @deansheather @bpmct @matifali