-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/warn user on archived GitHub repos #244
Conversation
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.
@snim2 I'm not seeing the warning. It looks like it's because that API requests are going to the incorrect URLs, e.g. it looks like it's going to https://api.github.com/repos/git@github.com:dxw/ht-body-exporter.git
What does your JSON file look like? I have: {
"plugins": [
{"name": "ht-body-exporter", "src": "git@github.com:dxw/ht-body-exporter.git"}
]
} and I get: ❯ whippet-test deps update
[Updating plugins/ht-body-exporter]
[Checking plugins/ht-body-exporter]
WARNING: GitHub repo is archived. This dependency should be replaced before the repo is removed.
HEAD is now at 3053e2e Initial commit
#############################################
# #
# WARNING: No inspections for this plugin #
# #
#############################################
|
This is by whippet.json:
I used bikeshed's whippet file as an example. Whether I use the http or ssh reference for the archived repo, I get the same result. I've put in logging to confirm that the curl request is made, but it's getting 404-ed. e.g. (with my logging included)
|
So, this is what I get with that file: ...
[Checking plugins/ht-body-exporter]
WARNING: GitHub repo is archived. This dependency should be replaced before the repo is removed.
HEAD is now at 3053e2e Initial commit
... and it still works if I copy over the Bikeshed files and add the new dependency. I'll investigate further... |
Cannot reproduce with PHP 8.2 or 8.3 |
OK, so somehow I can reproduce the error for several repos but not this one... |
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.
This is working for me now, for both ssh
and https
repo references.
One non-blocking suggestion: do we want to format the warning along the lines of the "No inspections for this plugin" warning, so it's less easily missed?
Yes, I think that's very wise!!! |
New look:
|
Squashing before merge |
This commit issues a warning to the user if they clone or checkout from an archived GitHub repository. In dxw, archiving a GitHub repository containing a dependency is likely to mean that the repository will soon be removed, and so we need to make users aware of this. However, we do not want to cause an error here, because we do not know whether Whippet is being run in the context of a local development environment or as part of a deploy process. Note that because we are only enabling this feature for a specific dxw use case, this commit does not make the effort to abstract out GitHub-specific code in a way that would make it easy to generalise the feature for GitLab, BitBucket, etc. This can be done in a future PR, if anyone needs it, but ideally we would want to improve the unit tests before hand.
b038b8e
to
08b4673
Compare
Warn user if they pull from an archived GitHub repo
This commit issues a warning to the user if they clone or checkout from an
archived GitHub repository. In dxw, archiving a GitHub repository containing
a dependency is likely to mean that the repository will soon be removed, and
so we need to make users aware of this. However, we do not want to cause
an error here, because we do not know whether Whippet is being run in the
context of a local development environment or as part of a deploy process.
Note that because we are only enabling this feature for a specific dxw use case,
this commit does not make the effort to abstract out GitHub-specific code in
a way that would make it easy to generalise the feature for GitLab, BitBucket,
etc. This can be done in a future PR, if anyone needs it, but ideally we would
want to improve the unit tests before hand.
Testing
Create a
whippet.json
file with an archived repository, for example:and run
whippet deps update
. Check that a warning is printed.Remove the lockfile and create a new
whippet.json
with a non-archived reposistory, e.g.:and run
whippet deps update
. Check that a warning is not printed.