hpi-swa / smalltalkCI

Framework for testing Smalltalk projects with GitHub Actions, GitLab CI, Travis CI, AppVeyor, and others.
MIT License
94 stars 68 forks source link

[improvement] now register the tested repository in iceberg for P9+ #580

Closed hogoww closed 1 year ago

hogoww commented 1 year ago

Clean redo of #579 fixing #552 ! I've enabled the registering of the repository for Pharo 9+ (therefore 10 and 11 at this time).

Pierre

hogoww commented 1 year ago

I like the idea! :)

While the change looks good, I would like to have it wrapped in an error handler. SmalltalkCI might run on anything, we can't be certain that this will be a valid Pharo repository. If it's not, the tests should still be run.

I'd have to check, but I think as long as it's a valid Monticello repository, it's valid for Iceberg. Would that be a fair assumption to have ? Still, I think the error handler is a good idea regardless.

I also don't like the override too much. I think it would make sense to have an afterLoad method that can be overridden and would do error handling automatically.

I like that too ! Will do :)

Another thing I was planning to add in a next PR would be to add the SmalltalkCI repository to Iceberg aswell, in the pharo execution script. That'd be easier for other purposes (such as creating a PR !)

Pierre

theseion commented 1 year ago

Another thing I was planning to add in a next PR would be to add the SmalltalkCI repository to Iceberg aswell, in the pharo execution script.

Might be a bit harder though, since you don't know where that repo is in relation to the one you're testing. Or do you mean to add it as a new clone?

hogoww commented 1 year ago

Actually, since you start from a Pharo script, it's fairly easy. That is, if my quick morning investigation was done properly ;)

theseion commented 1 year ago

Great then :)

hogoww commented 1 year ago

Icerberg seems to commit metadata changes silently... Anyway, should be fine now. @theseion does that fit your requirements? I wouldn't be against a better error message IMO.

theseion commented 1 year ago

@fniephaus You can merge this PR, unless you want to wait for @gcotelli's review.