-
Notifications
You must be signed in to change notification settings - Fork 21
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
DRAFT: Generates verification file for Chocolatey #79
base: main
Are you sure you want to change the base?
Conversation
Are you sure this is necessary? My understanding was that it was required when shipping binaries to Chocolatey, but |
@nex3 this is the information I received from their moderators. I can try to get more info, so they can provide a technical explanation for this requirement. |
That would be good—as far as I know we've been successfully releasing Sass without a verification file. |
@nex3 yes I had done a successful submission also that is why it seemed like it changed. I will try to get more info from them… |
@nex3 I have some more info, seems they define this type of package as "embedding the binaries" as per this link https://docs.chocolatey.org/en-us/create/create-packages#including-the-software-installer-in-the-package Also, my guess what happens with Sass has a "Trusted package" status, which means it gets approved after automated checks, and won't go through moderation anymore. I have asked some follow-up questions and can update once I hear back from them. |
It sounds like that section is talking about embedding binaries, where |
@nex3 This is their "official" response. I don't think I will push further.
If you think this merge is not needed, maybe there is a way to just bring in a verification file, even though you won't have the "dynamic" approach to it, maybe that is a workaround. What do you think? |
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.
Okay, in that case it sounds like this is necessary even if we aren't embedding binaries. Can you also add a test that the file is being created correctly?
@nex3 I have done the test, but was not able to run it locally. Getting the same error as the CI, but seems it was happening even before adding the test. |
It looks like the tests are failing because you're referring to Rather than always using the GitHub repo, it might make more sense to add a |
Have an implementation Will check why some tests are failing soon. |
Tests should be fixed by #81.
I don't know if it's worth allowing people to override the entire verification file, but I think having |
@leoafarias Are you planning on circling back to this? |
Yes, haven’t had much time this week. |
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.
Looks like a handful of tests are still failing.
Since you're adding new config variables, there are a few additional steps to go through:
- Write tests that verify that their default values are correct and that the overridden values are used in the verification file.
- Update the task documentation in
doc
to indicate thatpkg-chocolatey
depends onpkg.chocolateyRepoUrl
andpkg.chocolateyReleaseUrl
.
/// This is used for the Chocolatey verification file. | ||
/// It defaults to [githubRepo]. | ||
final chocolateyRepoUrl = InternalConfigVariable.fn<String>( | ||
() => 'https://github.com/${githubRepo.value}', |
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.
If the user hasn't set chocolateyRepoUrl
, they may be confused by the fact that they get an error about GitHub even if they aren't using GitHub deployment at all. It's probably better to explicitly fail with a message that says "pkg.chocolateyRepoUrl or pkg.githubRepo must be set to deploy to Chocolatey."
Same for chocolateyReleaseUrl
.
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.
@nex3 I have limited time, but I would love to finish this out over this weekend. Do you mind pointing me at some examples of these tests as I am still becoming familiar with the inner workings of the package?
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.
The GitHub configuration tests are a pretty good place to look:
dart_cli_pkg/test/github_test.dart
Line 41 in bf2815a
group("repo name", () { |
A verification file is now needed since this cli_pkg builds an embedded package with the software included inside the package.
Have successfully passed the approval Chocolatey approval process with the dynamically generated file in this PR.