List public networks as a regular user

[xuhang57]

Description

Currently, only the admin user can list networks. This patch adds the functionality that allows a regular user to list all public networks. Any feedback and suggestion of code revise is more than welcome.

The definition of public networks:

1. Public networks with which has no project associated.
2. Networks associated with the user’s Projects.
3. Networks associated with the projects that the user is added.

This PR fixes: https://github.com/CCI-MOC/hil/issues/765

Checklist before merging Pull Requests

• [x] New test(s) included reproducing the verify the feature
• [x] CI
• [x] Documentation
• [ ] Reviewed and approved by at least two other contributor.

[xuhang57]

I haven't finished reading all the migrations/fixtures. But I don' think we have a regular user created in the migrations for testing purpose. It would be nice if someone can confirm this. Thanks!

[zenhack]

I think we should limit the definition of public networks to item (1) in your list. We could maybe add a separate call list_project_networks. I think that would make the implementation much simpler.

I especially don't like trying to detect which projects a user has access to inside the call. We don't do this anywhere else; making the user specify a project would be more consistent, for one.

[naved001]

I especially don't like trying to detect which projects a user has access to inside the call.

Sorry but I don't understand what's wrong with it? I thought we didn't want HIL to have too many redundant APIs, and I figured that by doing auth checks we can list accessible networks.

[xuhang57]

This is a design problem then. If we have a convention, I will surely follow it. If there is not, it is a great time for us to discuss and make one, I believe.

Personally, I don't have a strong opinion on this. Basically, two choices:

• Minimal Design: Many functionalities per function.
• One functionality per function.

[zenhack]

It's a signficant departure from how all of the other APIs work, so that alone I think is a decent argument. But also, the current implementation's code is signficantly more fiddly than I would expect from the solution I propose. (@xuhang57 has some comments re: refactoring the loops, which I do think can be improved, but the implementation is needlessly complicated by the design).

[naved001]

Okay, I'll defer to you on this. So, list_networks will list all networks if run by admins, and only public networks if run by regular users. Then list_project_networks will list all networks owned by the project or all networks that are accessible by that project?

[zenhack]

Let's do accessible networks.

[xuhang57]

Great, I just pushed the change based on you guys' suggestions. list_project_networks has already been implemented.

I will work with Naved on the testing for the next step.

[xuhang57]

test added. Is the unit test the only test I need to worry about? (Most likely)

[xuhang57]

As for the docs/rest_api.md, please let me know if there is a required format. I try to make as few changes as possible.

[naved001]

@xuhang57 Just keep it consistent with how the other APIs are documented.

[xuhang57]

@naved001 @zenhack done.

[xuhang57]

@SahilTikale @naved001 @Izhmash @zenhack any feedback would be much appreciated

[xuhang57]

will rebase with the master

[xuhang57]

are we using squash merge? Maybe we can change the settings and make it as the default merge option.

[naved001]

we just do the regular merge.

[xuhang57]

@zenhack Done.

[xuhang57]

Do not merge yet since @Izhmash has a PR that enforcing a new format of writing testing scripts. Will pull his code and change this PR accordingly once his PR is merged.

[xuhang57]

@zenhack done. please merge if you think it is ok. Thanks!

[xuhang57]

@zenhack done.

[zenhack]

LGTM, merging.