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

cmta new command #237

Merged
merged 8 commits into from
Jul 18, 2023
Merged

cmta new command #237

merged 8 commits into from
Jul 18, 2023

Conversation

thinkmorestupidless
Copy link
Contributor

Testing with

cmta/run new -t lunatech-scala-2-to-scala3-course

The new command just prepares the correct options for, and delegates to, the install command...

This is still WIP as I need to ensure the zip file is extracted to a temp directory before the directory is renamed - in the case of name conflicts with existing versions of the same course.

When puling the Source Code release artifact the directory name is appended with the short commit hash of the release commit but still, if someone wants to create multiple scala courses, there'd be conflicts... we can either fail and require them to pass a new name or we can try to do it automatically and i'm taking a stab at doing it automatically.

@thinkmorestupidless thinkmorestupidless marked this pull request as draft July 7, 2023 11:53
@eloots eloots self-requested a review July 7, 2023 12:26
Copy link
Collaborator

@eloots eloots left a comment

Choose a reason for hiding this comment

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

Nice! Great enhancement. Just tried out both variants of the command (with and without the GitHub org prefix).

Copy link
Collaborator

@eloots eloots left a comment

Choose a reason for hiding this comment

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

The only issue I see is that the name of the folder of the new repo has some extra bit appended to it.

So for example, lunatech-labs-lunatech-scala-2-to-scala3-course-20510a2 instead of lunatech-labs-lunatech-scala-2-to-scala3-course.

I also like the prefixing with the GitHub org which makes it distinct from a studentified artifact that gets/was generated via cmtc install.

@thinkmorestupidless
Copy link
Contributor Author

The only issue I see is that the name of the folder of the new repo has some extra bit appended to it.

So for example, lunatech-labs-lunatech-scala-2-to-scala3-course-20510a2 instead of lunatech-labs-lunatech-scala-2-to-scala3-course.

I also like the prefixing with the GitHub org which makes it distinct from a studentified artifact that gets/was generated via cmtc install.

The postfix string is the hash of the commit of the release... it's not added by me but by github... it's easy enough to remove and i'll get to that.

I'll create a separate directory for 'cmta' courses, then, i'll have a think about what naming/location makes most sense

@eloots
Copy link
Collaborator

eloots commented Jul 10, 2023

The postfix string is the hash of the commit of the release

Sorry for missing that in your original message!

Yes, separating the main repos from the installed (studentified) courses make a lot of sense.

- Refactor `ctma new` and `cmtc install` implementations to
  not utilise the Github API & access token to retrieve
  artefacts and [release] tags
- test code needs to be refactored as it doesn't compile after
  the refactoring of the main code
- Left a couple TODO that require follow-up
val tag = githubProject.tag
val cloneGit = Process(Seq("git", "clone", s"[email protected]:$organisation/$project.git"), tmpDir)
val cloneGh = Process(Seq("gh", "repo", "clone", s"$organisation/$project"), tmpDir)
// TODO CHECK: I wonder if we should support cloning via HTTP...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see why we shouldn't - i think there are many people NOT using ssh so this would be necessary for them to use CMT

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed; There's corner case where the clone is done from a private repo. In that case, cloning via HTTP triggers a request from git for user credentials (via stdin/stdout). Then git reports that support for password authentication on HTTP is no longer supported. In any case, it should work without a problem for public repo's which covers a large chunk of the use cases. Also, the order in which the cloning is tried is ssh, gh (command), and http.
I'll run a few more experiments to figure out more details.

Comment on lines 68 to 70
// TODO This may be brittle; if the user doesn't have its git credentials set, we
// may want to retry using HTTP
// if it fails, we may still attempt to download the artefact
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a good point but I don't think it's something we should worry about for the 2.0 release - let's just create an issue for it and link to the issue in the TODO

Copy link
Collaborator

Choose a reason for hiding this comment

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

We may take another route. Instead of trying to figure out if there's an error in the template parameter, we could limit the implementation to simply try the download. We wouldn't be able to provide detailed error info in case of an error (such a non-existing release tag), but I don't think that's a big deal as the template parameter will be supplied by the course organiser, so if it's copy/pasted, there shouldn't be an issue.

If this approach is taken, the whole retrieval of the release tags would be removed from the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for creating an issue for changing the approach later.

eloots and others added 3 commits July 14, 2023 16:13
- Implement `cmtc install` from a folder
- Fix errors in `cmta new`
- Added checks for pre-existing targets
@eloots eloots marked this pull request as ready for review July 17, 2023 16:07
@eloots eloots merged commit 08d313f into main Jul 18, 2023
3 checks passed
@eloots eloots deleted the cmta-new branch July 18, 2023 09:20
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