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

Clone without virtualenv #462

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Alcolo47
Copy link
Contributor

Create working directory in /tmp without creating virtualenv.
Then we can run tests within current environment (environment where cosmic-ray was launched from).
For this, multiple Workspace classes was created and workspace instantiation is driven with a new parameter [cloning] workspace_type

@abingham
Copy link
Contributor

Then we can run tests within current environment (environment where cosmic-ray was launched from).

Why do we need to do this?

@Alcolo47
Copy link
Contributor Author

Because, for my project in don't need to deploy my sources with setup.py or anything else.
My working environment is fully operational to run tests. Then:

  • I spent 2/3 minutes to reinstall all /tmp virtualenvs * (cpu core number ==4)
  • This can fails because there is to much pip concurrency on pypi.org access (timeout can occurs)
    This, to duplicate an already existing environment.
  • And in my stack gitlab ci / dockerizer runner, I have a problem by mixing virtualenv and pipenv.

@abingham
Copy link
Contributor

abingham commented Oct 1, 2019

Ok, that's reasonable justification for this feature. I would, however, like to simplify the implementation. The use of metaclasses is a bit over-engineered. I think we could replace it and the get_workspace_class() and get_workspace() classmethods with a much simpler module-scope function (e.g. get_workspace()) that returns an instance.

As it is, the use of the metaclass means I have to think harder than necessary about the code to come to the conclusion that it's just a complicated way to dispatch between two alternative workspace classes.

@Alcolo47
Copy link
Contributor Author

Alcolo47 commented Oct 1, 2019

Yes, today it's overkill.

I have in my brain a others type of workspaces:

  • docker
  • docker swarm
  • Kubernete (but I not competent for this now)

I wrote anything about that yet, but a real factory mechnanism seems to me a good start.

Other factories possibilities are:

  • With a register class decorator (Writing decorator is as readable as metaclasses).
  • Using plugin system, but today, workspace api is too simple for all of those workspaces. Then giving the possibility to write a 3rd party's plugin is not desirable.

@abingham
Copy link
Contributor

abingham commented Oct 2, 2019

Using plugin system,

If there's really a need for many different kinds of workspaces, this is definitely my preference. It's how the rest of CR works, and it seems to work very well.

today, workspace api is too simple for all of those workspaces

Right. In my experience the hard part of defining a new plugin type is figuring out what the proper abstraction/API is. It sounds like you've identified 4 or 5 examples, and that should be enough for us to start to design a reasonable plugin API for workspaces.

Then giving the possibility to write a 3rd party's plugin is not desirable.

Maybe I misunderstand what you're saying, but I think letting 3rd parties write workspace plugins is very desirable.

@Alcolo47
Copy link
Contributor Author

Alcolo47 commented Oct 2, 2019

Maybe I misunderstand what you're saying, but I think letting 3rd parties write workspace plugins is very desirable.

It is very desirable is the base API is well defined. But it is not the case yet.

@abingham
Copy link
Contributor

abingham commented Oct 2, 2019

It is very desirable is the base API is well defined. But it is not the case yet.

Good, then we're in agreement :)

@abingham
Copy link
Contributor

Can I close this in favor of defining a plugin system for workspaces?

@@ -123,6 +123,11 @@ def badge_format(self):
def badge_thresholds(self):
return self.badge['thresholds']

@property
def cloning_config_workspace_type(self):
"The 'workspace' section of the cloning section"
Copy link

@mmphego mmphego Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"The 'workspace' section of the cloning section"
"""The 'workspace' section of the cloning section."""

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.

3 participants