-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add ICAT Backend Documentation #192
Add ICAT Backend Documentation #192
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.
On the whole I thought it was really well written, and I liked the use of external links and example commands.
There were one or two things I couldn't get to work, I don't know whether the root cause is elsewhere or if it's say a missing command from the documentation, but I thought it best to mention it at least. If it definitely should work based on the commands given and it's just a mistake on my end then that's fair enough.
@@ -120,13 +145,16 @@ intricacies of this command: | |||
poetry add [PACKAGE-NAME] | |||
``` | |||
|
|||
### Nox (Automated Testing & Other Code Changes) |
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.
Couldn't get this section to work, running nox -s black
, nox -s lint
, nox -s safety
all fail with variations on the same error message:
ERROR: In --require-hashes mode, all requirements must have their versions pinned with ==. These do not: flake8 from https://files.pythonhosted.org/packages/d4/ca/3971802ee6251da1abead1a22831d7f4743781e2f743bd266bdd2f46c19b/flake8-3.8.4-py2.py3-none-any.whl#sha256=749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839
I notice that hashes are provided in poetry.lock
, so should the methods in noxfile.py
pin versions? Obviously this might not be an issue with the documentation per se, I wasn't sure if there's a step here I should be doing that's not explicitly mentioned or if this is a bug in its own right.
For what it's worth, running nox -s tests
didn't have this issue (it runs poetry install
rather than poetry export
/pip isntall
.
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.
It's difficult to know for sure what issue you might be facing, but this is an interesting issue, I wonder if the bit about adding --without-hashes
to the poetry export
commands might work: python-poetry/poetry#3472 (comment)
What do you mean by pinning versions in the functions in noxfile.py
?
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.
Adding --without-hashes
here works like you said (I also noticed it's already present in another command further down).
I had thought another way round it would be to include the hashes in the pip install
command which as far as I can tell is generated here. For example replacing "flake8"
with "flake8==3.7.9 "
, which is what the 4th bullet point in the comment you link suggests. But since the --without-hashes
method seems to work (and seems simpler) I guess it'd be better to go with that?
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.
Thanks for testing that, I've committed that change, as per commit fb3900b. Yeah that would be another way around the issue but I guess that means managing versions within the noxfile and within Poetry - not the end of the world since they're dev dependencies, but still nice to avoid. Hopefully this is now resolved.
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.
Great, as long as there aren't any unforseen consequences or anything to adding in --without-hashes
then I'd agree that this seems like the best solution.
- `helpers.py` - Helper functions that are called in `backend.py` | ||
|
||
|
||
### Creating a Backend |
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.
So I'm not 100% clear on this part, the documentation seems to imply that the backend will be created for me when running main.py
, but I could only get it to load the swagger interface when I gave it the pre-prod url ("ICAT_URL": "https://scigateway-preprod.esc.rl.ac.uk:8181"
). I suppose that I've misunderstood and that the API is created for me, but I would need to have actually created a database (or python backend) myself?
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 concept of a backend is only a developer concept and isn't something that a user needs to know about. You should've been able to do something like the following though?
curl --request POST 'http://127.0.0.1:5000/sessions' --header 'Content-Type: application/json' --data '{
"username": "root",
"password": "pw",
"mechanism": "simple"
}'
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.
Yeah I was able to submit requests through the swagger interface (through browser) or with curl
after setting "ICAT_URL": "https://scigateway-preprod.esc.rl.ac.uk:8181"
. My confusion was with whether I'd missed a step and should be say setting something up to run on localhost:8181
while following the documentation and then provide that in the config file or whether someone following the documentation would already have an ICAT_URL instance set up for them and they only need to modify the config file. I guess from your response it's the latter?
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.
I would imagine a 'typical' user would probably have an ICAT_URL already so they only need to modify the config file. As a developer, I have an ICAT instance setup locally which runs on localhost:8181
(I used some tutorials on the icat.manual repo) but any form of prod/pre-prod would use a well-established ICAT URL. I've added a link to these tutorials in the README, as per commit 73bb72d
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.
That makes sense, and I think the extra explanation helps clarify it (though I do appreciate that typical users and experienced devs probably wouldn't need to be explicitly told this).
- This has been removed in a different branch so I've removed the documented details about it in this branch and adjusted it according to the new solution found
- This commit also adds the "tests" session to the list of Nox sessions
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.
I'm happy with with the small changes to areas I didn't understand or couldn't get to run the first time round. While the additions might not be necessary for someone who knows what they're doing, as someone who's looking at this for the first time they helped me understand.
@@ -120,13 +145,16 @@ intricacies of this command: | |||
poetry add [PACKAGE-NAME] | |||
``` | |||
|
|||
### Nox (Automated Testing & Other Code Changes) |
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.
Great, as long as there aren't any unforseen consequences or anything to adding in --without-hashes
then I'd agree that this seems like the best solution.
- `helpers.py` - Helper functions that are called in `backend.py` | ||
|
||
|
||
### Creating a Backend |
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.
That makes sense, and I think the extra explanation helps clarify it (though I do appreciate that typical users and experienced devs probably wouldn't need to be explicitly told this).
This PR will close #190
Description
This PR makes quite a lot of change to
README.md
for the changes I've made to the API over the past few months. I've also cleaned up what was in the file beforehand.There's a commit related to a suggested change which should be made in. I made the change in this branch for speed and simplicity, as explained: #185 (comment)
I've also renamed
query_filter.py
toquery_filter_factory.py
to make prevent any confusion.Testing instructions
Agile board tracking
connect to #190