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

Update pip config #607

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Conversation

MarkSymsCtx
Copy link
Contributor

I'm currently running some tests on this to check it's not regressed anything

rosslagerwall
rosslagerwall previously approved these changes Mar 7, 2023
edwintorok
edwintorok previously approved these changes Mar 7, 2023
drivers/VDI.py Outdated Show resolved Hide resolved
drivers/VDI.py Outdated Show resolved Hide resolved
@ydirson
Copy link
Contributor

ydirson commented Mar 7, 2023

I think we should keep the dev_requirements.txt, it is really much easier to start working on the code by running pip in a venv, than having to lookup in the github actions which packages to install and do so manually.

TimSmithCtx
TimSmithCtx previously approved these changes Mar 8, 2023
@MarkSymsCtx
Copy link
Contributor Author

I think we should keep the dev_requirements.txt, it is really much easier to start working on the code by running pip in a venv, than having to lookup in the github actions which packages to install and do so manually.

We have a corporate security directive to avoid use of pip in general and source repositories are regularly scanned to nag about the presence of pip configuration information. These files got missed in earlier clean up activities.

@ydirson
Copy link
Contributor

ydirson commented Mar 8, 2023

We have a corporate security directive to avoid use of pip in general and source repositories are regularly scanned to nag about the presence of pip configuration information.

Would it more acceptable in a setup.cfg ?

@MarkSymsCtx
Copy link
Contributor Author

We have a corporate security directive to avoid use of pip in general and source repositories are regularly scanned to nag about the presence of pip configuration information.

Would it more acceptable in a setup.cfg ?

No, they get complained about as well. Basically anything with pip is deemed to be insecure and a problem with supply chain verification.

@ydirson
Copy link
Contributor

ydirson commented Mar 9, 2023

We have a corporate security directive to avoid use of pip in general and source repositories are regularly scanned to nag about the presence of pip configuration information.

Would it more acceptable in a setup.cfg ?

No, they get complained about as well. Basically anything with pip is deemed to be insecure and a problem with supply chain verification.

I'm sorry corporate directives impair ability for devs inside Xenserver to work efficiently (even more so as I've endured such rules in past jobs), but surely we can find a way to provide dependency information in a standard machine-parsable way a public project?

@MarkSymsCtx
Copy link
Contributor Author

We have a corporate security directive to avoid use of pip in general and source repositories are regularly scanned to nag about the presence of pip configuration information.

Would it more acceptable in a setup.cfg ?

No, they get complained about as well. Basically anything with pip is deemed to be insecure and a problem with supply chain verification.

I'm sorry corporate directives impair ability for devs inside Xenserver to work efficiently (even more so as I've endured such rules in past jobs), but surely we can find a way to provide dependency information in a standard machine-parsable way a public project?

It doesn't actually impair us at all as we use the RPM build mock to run this stuff and that just derives everything it needs from the specfile.

@ydirson
Copy link
Contributor

ydirson commented Mar 9, 2023

But then, not providing machine-parsable dependency info is impairing the community - providing them to the community does not force their internal use.

@gwd
Copy link

gwd commented Mar 9, 2023

@MarkSymsCtx As an open-source project under the umbrella of the Xen Project, it's important that there be a clear, practical way for external contributors to build and contribute xapi. Is such a method documented anywhere?

Also, are there any other upstream projects that XenServer either consumes or contributes to, which have setup.cfg or dev_requirements.txt files? How are those dealt with WRT the corporate anti-pip policy?

@ydirson Would it be practical for you to use mock to do building as well? Or alternately, would a Docker file or a vagrant file with the relevant dependencies be suitable?

@ydirson
Copy link
Contributor

ydirson commented Mar 9, 2023

Would it be practical for you to use mock to do building as well? Or alternately, would a Docker file or a vagrant file with the relevant dependencies be suitable?

Mock is useful for build reproducibility of RPMs, but before we're at rpm-building stage we should be able to easily run unit tests, which can only run when we have the proper dependencies installed. Sure, the SM build scripts today ensure that unit tests and even pylint are run in the mock environment on each build, but this is much too costly for everyday use, for example when working on a specific failed test.

Further more, allowing users to use pip makes it easy to test with different versions of dependencies and checker tools (in separate python virtual environments), which in turn allows to further improve the project quality.

@MarkSymsCtx
Copy link
Contributor Author

Fine requirements file reinstated but we will not be maintaining it or keeping it updated (not least because we have no environment in which we could test that it is valid and/or useful) so it will most likely become stale quite quickly.

@ydirson
Copy link
Contributor

ydirson commented Mar 9, 2023

Keeping it used by github actions would likely be the best option, that would ensure the info in there is still tested - and would avoid getting it duplicated in 2 places.

@MarkSymsCtx MarkSymsCtx changed the title Remove pip config Update pip config Mar 9, 2023
drivers/blktap2.py Outdated Show resolved Hide resolved
Signed-off-by: Mark Syms <mark.syms@citrix.com>
drivers/SRCommand.py Outdated Show resolved Hide resolved
Signed-off-by: Mark Syms <mark.syms@citrix.com>
@MarkSymsCtx MarkSymsCtx requested review from edwintorok, rosslagerwall, ydirson and TimSmithCtx and removed request for ydirson March 10, 2023 11:02
@ydirson
Copy link
Contributor

ydirson commented Mar 10, 2023

I'm surprised when running pylint locally (fresh venv on CentOS7, populated with same commands as github actions):

  • when including tests/mocks/ in PYTHONPATH as the github actions do, I get 7 occurrences of Module 'XenAPI' has no 'xapi_local' member. git grep XenAPI.xapi_local shows them all (and more, which show that the files not named *.py are apparently skipped by pylint).
  • aside from those I also have drivers/on_slave.py:130:4: E0401: Unable to import 'XenAPIPlugin' (import-error)

Any idea why those errors don't appear in the CI ?

@MarkSymsCtx
Copy link
Contributor Author

I'm surprised when running pylint locally (fresh venv on CentOS7, populated with same commands as github actions):

  • when including tests/mocks/ in PYTHONPATH as the github actions do, I get 7 occurrences of Module 'XenAPI' has no 'xapi_local' member. git grep XenAPI.xapi_local shows them all (and more, which show that the files not named *.py are apparently skipped by pylint).
  • aside from those I also have drivers/on_slave.py:130:4: E0401: Unable to import 'XenAPIPlugin' (import-error)

Any idea why those errors don't appear in the CI ?

Unfortunately not, if you do get to the bottom of please send an update PR to rectify the issues.

@MarkSymsCtx
Copy link
Contributor Author

MarkSymsCtx commented Mar 14, 2023

Actually, the xapi_local complaint is probably because https://github.com/xapi-project/sm/blob/master/tests/mocks/XenAPI/__init__.py does indeed not have the xapi_local member. That code should actually get moved to a tests/mocks/XenAPI.py file and have the xapi_local method declared as a no-op for testing purposes.

Although actually making this change makes the unit tests fail through a lack of coverage.

@ydirson
Copy link
Contributor

ydirson commented Mar 17, 2023

XenAPI/init.py does indeed not have the xapi_local member.

Yes, that's why my question was why it indeed works on the CI, and I'm still in the dark there.

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.

6 participants