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.

Sign up for GitHub

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

Jump to bottom

Remove custom virtual env #9570

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Remove custom virtual env #9570

merged 1 commit into from
Mar 17, 2021

Conversation

nixocio
Copy link
Contributor

@nixocio nixocio commented Mar 11, 2021 •
edited
Loading

Remove custom virtual from the UI.

Also, surface missing-resource warnings on list items for UJTs that were using
custom virtualenvs. And related details page.

See: #9190
Also: #9207

@nixocio
Copy link
Contributor Author

nixocio commented Mar 11, 2021

@tiagodread @unlikelyzero this still a working in progress, but I will run the e2e tests. I believe this one will disrupt a lot tests.

@one-t one-t self-assigned this Mar 11, 2021
@nixocio nixocio force-pushed the ui_issue_9190 branch 5 times, most recently from 250eb3e to 99c7beb Compare March 15, 2021 15:55
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@nixocio nixocio marked this pull request as ready for review March 15, 2021 17:02
@nixocio
Copy link
Contributor Author

nixocio commented Mar 15, 2021

Adding more unit-tests.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@nixocio nixocio requested a review from jakemcdermott March 15, 2021 20:14
@one-t
Copy link
Contributor

one-t commented Mar 15, 2021

Just some testing updates-
I gave this a spin by adding a custom venv to a job template via psql commands

UPDATE main_jobtemplate
SET custom_virtualenv = '/opt/my-venvs/custom-venv';

image

I did find that the custom venv paths option is still present in system settings:
image

Otherwise, this looks good to me. Once that is removed from the system settings page we should be good to merge.

ryanpetrello
ryanpetrello reviewed Mar 16, 2021
View reviewed changes
awx/ui_next/src/components/ExecutionEnvironmentDetail/ExecutionEnvironmentDetail.jsx
<span>
<Tooltip
content={i18n._(
t`Custom virtual environment ${virtualEnvironment} must be replaced by an execution environment.`
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @shanemcd @wenottingham @tvo318 we may want to come up with some instructions (at some URL) for doing so - I have to imagine we'll have some type of knowledge base article or documentation that describes the transition process - do we want to file an issue to track this work so we don't forget it later?

@nixocio
Copy link
Contributor Author

nixocio commented Mar 16, 2021

Just some testing updates-
I gave this a spin by adding a custom venv to a job template via psql commands

UPDATE main_jobtemplate
SET custom_virtualenv = '/opt/my-venvs/custom-venv';

image

I did find that the custom venv paths option is still present in system settings:
image

Otherwise, this looks good to me. Once that is removed from the system settings page we should be good to merge.

image

image

image

I updated the settings. Good find @one-t. If you want to take another look.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@one-t
Copy link
Contributor

one-t commented Mar 16, 2021 •
edited by nixocio
Loading

That looks good- I do have a couple other points to mention, though they may be less common.

  • Custom Venvs can be defined in three places, with increasing precedence. (Org, Project, JT) Do we want to display warnings on job templates and projects that inherit their custom venv from one of these resources?
  • This warning on a org or project with a venv defined should possibly be updated to indicate that there is a custom venv defined rather than saying it's missing an EE since it is inherited from the org.
    image
    • this one is kind of tricky because you can't unset the custom venv in the UI.

Since custom venvs are already a power-user thing, maybe we just add some documentation on how to remove the custom venvs via awx-manage?

Anyway, not really bugs at all, just potential UX issues.

@nixocio
Copy link
Contributor Author

nixocio commented Mar 16, 2021 •
edited
Loading

Hey @one-t, we do show warnings about missing EE on lists for - Orgs, Projects, Inventory Sources, and Job Templates. We also show warnings on details pages for those resources.

image

As we talked, based on your feedback I will make the message on the list to have the custom virtual env to be replaced as shown on the details page. That is a good suggestion.

@nixocio
Remove custom virtual env
babea5d 
Remove custom virtual from the UI.

Also, surface missing-resource warnings on list items for UJTs that were using
custom virtualenvs.

Fix some uni-tests warnings.

See: ansible#9190
Also: ansible#9207
@nixocio
Copy link
Contributor Author

nixocio commented Mar 16, 2021 •
edited
Loading

@one-t, updated. Now the same message is displayed on the lists and details. I triggered the e2e tests again to verify the latest changes.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 6b1110f into ansible:devel Mar 17, 2021
@tiagodread tiagodread mentioned this pull request Apr 5, 2021
Closed
This was referenced Jan 19, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanpetrello ryanpetrello ryanpetrello left review comments

@one-t one-t one-t approved these changes

@jakemcdermott jakemcdermott jakemcdermott approved these changes

Assignees

@one-t one-t

Labels
component:ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nixocio @one-t @ryanpetrello @jakemcdermott