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

Submit without specifying files #1044

Merged

Conversation

andrerfcsantos
Copy link
Member

@andrerfcsantos andrerfcsantos commented Jun 7, 2022

Fixes #991

Adds the possibility of submitting without specifying files, with the "algorithm" specified in #991 (comment)

Other than the tests in the PR, I also tried to submit solutions using this version of the program, including messing up the exercise directory and everything went as expected :)

@asarkar
Copy link

asarkar commented Jun 7, 2022

This PR seems to assume the current directory is an exercise directory. As a student, I build and submit my projects from the workspace root, not from individual exercises. I should be able to do exercism submit <exercise_dir> from anywhere, since the CLI already knows the workspace path.

@andrerfcsantos
Copy link
Member Author

This PR does assume that.

I sympathize with your use case, but I think allowing directories should probably be discussed better outside this PR. The idea here is to allow to submit files without specifying anything. Allowing directories should be another feature and a separate discussion as the CLI currently explicitly disallows them.

@asarkar
Copy link

asarkar commented Jun 7, 2022

I don’t think it is “my use case” as much as it is convenience and common sense. If the point of this PR is to improve user experience, adding an additional step of going in the directory is anything but. If the goal of this PR is to reduce user error due to missing files, that is not achieved either because they may submit from a wrong working directory.
Without directory support, all this PR does is help close a ticket, not help a student.

@ErikSchierboom
Copy link
Member

I sympathize with your use case, but I think allowing directories should probably be discussed better outside this PR. The idea here is to allow to submit files without specifying anything. Allowing directories should be another feature and a separate discussion as the CLI currently explicitly disallows them.

Like in my comment above, the CLI has been carefully designed and has evolved over a long period of time. Therefore, new changes should be discussed before being implemented and carefully considered by the Exercism team.

I don’t think it is “my use case” as much as it is convenience and common sense.
If the point of this PR is to improve user experience, adding an additional step of going in the directory is anything but.

This is a personal opinion and not something that has any data to back it up with. Exercism documents and guides are all written from the viewpoint of the CLI being used from within the exercise's directory. This is also how the vast majority of users use the CLI. I also don't like the "common sense" remark, which suggests that arguing otherwise implies a lack of common sense.

Note that we're not against any changes, but we do feel that changes to such a widely-used tool as the CLI should be carefully thought out and considered by the Exercism team. You clearly feel strongly about being able to submit a directory, so what you should do is open an issue where we can discuss its merits.

If the goal of this PR is to reduce user error due to missing files, that is not achieved either because they may submit from a wrong working directory.

I disagree with this statement. For the vast majority of the users, this PR will be extremely helpful. It will make using the CLI more ergonomic, greatly reduces the chance of students submitting the wrong files (this will only happen if they manually submit files) whilst still allowing for the current behavior.

Without directory support, all this PR does is help close a ticket, not help a student.

This is a rude statement and not constructive in any way. I'll kindly refer you to our Code of Conduct for guidance on how we expect our community members to interact with each other.

Side note: I think the feature this PR supports are possibly the number one most request feature.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Thank you so much for doing this! This is probably our number one most wanted CLI feature :)

I don't know any Go though, so maybe @ekingery could take a look?

cmd/submit.go Show resolved Hide resolved
@iHiD
Copy link
Member

iHiD commented Jun 8, 2022

@andrerfcsantos Thanks for doing this!

Copy link
Member

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

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

This looks lovely. Clean, straight forward, and easy to follow. ❤️ Thank you for taking the time to implement this!

Copy link
Contributor

@ekingery ekingery left a comment

Choose a reason for hiding this comment

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

Excellent, thank you @andrerfcsantos!

@ErikSchierboom ErikSchierboom merged commit 8e73ec3 into exercism:main Jun 9, 2022
@ErikSchierboom
Copy link
Member

Merged! Thanks so much!

Next step: building a new release. @ekingery remind me what the steps are?

@ee7
Copy link
Member

ee7 commented Jun 9, 2022

remind me what the steps are?

There is RELEASE.md. The previous release was the first that was signed, with @ekingery's key. So I guess Eric has to be the person who releases, if we want to keep signing with the same key.

The process of verifying the signature is:

  1. Go to https://github.com/exercism/cli/releases
  2. Download exercism_checksums.txt and exercism_checksums.txt.sig to the same directory
  3. (Optionally, gpg --recv-keys EAF483E69A029F9E89AA97CD92CA296D5C8C4CD1, to import the key in a separate step)
  4. Run gpg --verify exercism_checksums.txt.sig

I don't know whether the CLI is packaged anywhere that expects releases to be signed with that key. At least the AUR package doesn't check the key. If it did, it would have this line:

validpgpkeys=('EAF483E69A029F9E89AA97CD92CA296D5C8C4CD1')

For later, should we consider having a dedicated key for signing CLI releases, which perhaps could live in this repo as a secret? See GoReleaser docs on signing.


A release would also be nice to include commits like:

Currently:

$ exercism version
exercism version 3.0.13
$ exercism --help | head -n1
A command-line interface for the v2 redesign of Exercism.

@ekingery
Copy link
Contributor

ekingery commented Jun 9, 2022

remind me what the steps are?

There is RELEASE.md. The previous release was the first that was signed, with @ekingery's key. So I guess Eric has to be the person who releases, if we want to keep signing with the same key.

The process of verifying the signature is:

  1. Go to https://github.com/exercism/cli/releases
  2. Download exercism_checksums.txt and exercism_checksums.txt.sig to the same directory
  3. (Optionally, gpg --recv-keys EAF483E69A029F9E89AA97CD92CA296D5C8C4CD1, to import the key in a separate step)
  4. Run gpg --verify exercism_checksums.txt.sig

I don't know whether the CLI is packaged anywhere that expects releases to be signed with that key. At least the AUR package doesn't check the key. If it did, it would have this line:

validpgpkeys=('EAF483E69A029F9E89AA97CD92CA296D5C8C4CD1')

For later, should we consider having a dedicated key for signing CLI releases, which perhaps could live in this repo as a secret? See GoReleaser docs on signing.

Yes, the generated packages should not rely on my (or any) signature at this point, so changing the signing key for the next release will be fine. I agree, a dedicated signing key (or keys) would be best - I just signed it because it was faster to get done so it's probably also fine to have whoever is building the release sign it. Ideally we would continue to automate the release process by building and signing using Github Actions: https://goreleaser.com/ci/actions/?h=sign#signing

@ErikSchierboom
Copy link
Member

@ekingery I think getting the release process automated is quite important. Would you be interested in working on that?

@ekingery
Copy link
Contributor

@ekingery I think getting the release process automated is quite important. Would you be interested in working on that?

I will spend a little time poking around - shouldn't be too much work to at least make some progress, although I can't promise anything due to a pretty tight schedule I'm working with these days. I am happy to work with anyone else who volunteers as well - anyone with some knowledge or interest in learning github actions and simple scripting tools would be able to contribute meaningfully.

@ErikSchierboom
Copy link
Member

@ekingery I'm happy to help with the GitHub Actions part, as I've worked with it quite a lot. Others can join too of course. Maybe someone from @exercism/github-actions?

@andrerfcsantos
Copy link
Member Author

@ErikSchierboom I can help. I'm no expert in Github Actions, but I can learn :) Let me know what you need.

@ErikSchierboom
Copy link
Member

Great! We have a RELEASE.md document that details how a release can be built using GoReleaser automatically.

Here is the page with details on integrating GoReleaser with GitHub actions: https://goreleaser.com/ci/actions/ Their example runs the script when a new tag is published, and I think that is a nice way to handle this.

@andrerfcsantos
Copy link
Member Author

@ErikSchierboom Will take a closer look at it later. However, at a quick glance, I see we'll need secrets.GPG_PRIVATE_KEY, secrets.PASSPHRASE and secrets.GITHUB_TOKEN defined. Do we have this already?

@ErikSchierboom
Copy link
Member

The secrets.GITHUB_TOKEN value is automatically provided by GitHub actions: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret

The other, we'll need to define ourselves. For the other two values, we'll need to set them manually I guess. @iHiD could you check whether there are any secrets defined for the GPG private key and/or passphrase? And if not, add them?

@andrerfcsantos
Copy link
Member Author

Cool!

Reading the release instructions more carefully, I realize we also need the credentials for snapcraft to publish the snap. This post mentions that it's possible to create a credentials file for snap. Putting this file as a secret will also be useful for CI to perform snapcraft login before publishing the snap.

@ErikSchierboom
Copy link
Member

Ah neat. I'll check that out.

@kytrinyx
Copy link
Member

@ekingery I'd also be happy to help work on the automation bit. Meanwhile... how hard would it be to do a manual release? I'd really like to get this change shipped so people can use it. I'd be happy to do the release if there's a way to do it without your keys, so long as I can holler if I get stuck working through RELEASE.md, since it's been a few years since I've touched this repo.

@andrerfcsantos andrerfcsantos deleted the submit-without-specifying-files branch September 27, 2022 08:51
@ekingery
Copy link
Contributor

@ekingery I'd also be happy to help work on the automation bit. Meanwhile... how hard would it be to do a manual release? I'd really like to get this change shipped so people can use it. I'd be happy to do the release if there's a way to do it without your keys, so long as I can holler if I get stuck working through RELEASE.md, since it's been a few years since I've touched this repo.

@kytrinyx, cutting a release manually would be a great way to prep for the automation. It should be much easier than it used to be 🤞. As you have probably seen it does require the new tooling as described in the RELEASE.md file. Happy to help with any issues you run into or questions you have! I am on the exercism slack, or we can hash things out here or via email - whatever works best for you.

@kytrinyx
Copy link
Member

Awesome, thank you @ekingery. I'll take a stab at this over the weekend.

@SleeplessByte
Copy link
Member

@kytrinyx when you eventually succeed, can you give me a ping with the version number so I can update the vscode extension to use the automated submission from that version forward?

@kytrinyx
Copy link
Member

kytrinyx commented Oct 4, 2022

Okay, I've cut a release but was unable to publish to snapchat (linux) and homebrew (mac). People should still be able to install the old version and then use the exercism upgrade command to get the newer version. Presumably. Maybe.

@SleeplessByte the new version is v3.1.0

@andrerfcsantos
Copy link
Member Author

@kytrinyx Can confirm upgrading with exercism upgrade works.

$ exercism version
exercism version 3.0.13
$ exercism upgrade
$ exercism version
exercism version 3.1.0

@kytrinyx
Copy link
Member

kytrinyx commented Oct 4, 2022

@andrerfcsantos That's excellent news! Thank you.

@ee7
Copy link
Member

ee7 commented Oct 4, 2022

Great to see a new release!

I checked verifying the signature with the new key:

$ cd /tmp || exit
$ curl -sSfLO 'https://github.com/exercism/cli/releases/download/v3.1.0/exercism-3.1.0-linux-x86_64.tar.gz'
$ curl -sSfLO 'https://github.com/exercism/cli/releases/download/v3.1.0/exercism_checksums.txt'
$ curl -sSfLO 'https://github.com/exercism/cli/releases/download/v3.1.0/exercism_checksums.txt.sig'
$ gpg --keyserver keys.openpgp.org --verbose --recv-key 816A8B8E703E53110F1D0FBBE8761A9E46258B62
gpg: data source: http://keys.openpgp.org:11371
gpg: armor header: Comment: 816A 8B8E 703E 5311 0F1D  0FBB E876 1A9E 4625 8B62
gpg: armor header: Comment: Katrina Owen <[email protected]>
gpg: pub  rsa4096/E8761A9E46258B62 2020-10-17  Katrina Owen <[email protected]>
gpg: using pgp trust model
gpg: key E8761A9E46258B62: public key "Katrina Owen <[email protected]>" imported
gpg: Total number processed: 1
gpg:               imported: 1
$ gpg --verify --verbose exercism_checksums.txt.sig
gpg: assuming signed data in 'exercism_checksums.txt'
gpg: Signature made Tue 04 Oct 2022 11:29:59 CEST
gpg:                using RSA key 816A8B8E703E53110F1D0FBBE8761A9E46258B62
gpg: using pgp trust model
gpg: Good signature from "Katrina Owen <[email protected]>" [unknown]
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 816A 8B8E 703E 5311 0F1D  0FBB E876 1A9E 4625 8B62
gpg: binary signature, digest algorithm SHA256, key algorithm rsa4096
$ sha256sum --check --ignore-missing exercism_checksums.txt
exercism-3.1.0-linux-x86_64.tar.gz: OK

I know at least the AUR package doesn't need to update the key, since it didn't check the signature anyway: https://aur.archlinux.org/cgit/aur.git/plain/PKGBUILD?h=exercism

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.

Allow submitting solution without specifying files
9 participants