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

Add repr filter #382

Merged
merged 2 commits into from Feb 9, 2022
Merged

Add repr filter #382

merged 2 commits into from Feb 9, 2022

Conversation

mdellweg
Copy link
Member

@mdellweg mdellweg commented Aug 5, 2020

No description provided.

@pulpbot
Copy link
Member

pulpbot commented Aug 5, 2020

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

@mdellweg mdellweg force-pushed the repr_filter branch 2 times, most recently from 789b6c5 to e778c98 Compare August 5, 2020 09:43
@mdellweg mdellweg changed the title WIP: Add repr filter Add repr filter Aug 5, 2020
@mdellweg mdellweg force-pushed the repr_filter branch 4 times, most recently from 2fa54a0 to 3a721af Compare August 5, 2020 14:54
@mdellweg
Copy link
Member Author

mdellweg commented Aug 5, 2020

I switched all the dynamic scenarios to ansible 2.9 now, as ansible 2.8 does not work there.

@mdellweg mdellweg marked this pull request as ready for review August 5, 2020 15:27
Copy link
Member

@fao89 fao89 left a comment

Choose a reason for hiding this comment

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

Glad to see it tested as collection

Makefile Outdated
@@ -0,0 +1,51 @@
NAMESPACE := $(shell python -c 'import yaml; print(yaml.safe_load(open("galaxy.yml"))["namespace"])')
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this Makefile! It is pretty cool!

@@ -1 +0,0 @@
../../roles
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Also, this entire change (or 2 changes) needs a ticket or 2 in pulp.plan.io.

Even if it only affects developers, it is a very large change. Only small changes that are presumed to not be visible to users can get away without a ticket.

Comment on lines 30 to 34
ansible: "2.9"
python: "2.7"
toxpy: "27"
- test_type: packages-dynamic
ansible: "2.8"
ansible: "2.9"
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment about the limitations of ansible 2.8.

Also, I want to have more than 1 test for python 3.8.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not see, where we document the dynamic use of the roles. That would be the perfect place.

Copy link
Member

Choose a reason for hiding this comment

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

@mdellweg We do not. I suppose we should create a sample playbook and page under docs, similar to the letsencrypt playbook/page I just created.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am tempted to say, if we never announced that "feature", we'd rather not talk about it now, so we never actually need to support it.

Copy link
Member

Choose a reason for hiding this comment

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

I feel there is a whole bunch of docs we should write for the installer.

For one thing, I want this to be a section of the docs:
https://pulpproject.org/2020/07/09/pulp-3.5-installer-roles/

We should address it at that time, since it is related (running only 1 role on a each host, compared to running roles 1 at a time on a single host.)

However, I want this limitation that we are aware of, and that bit us, to be captured somewhere at least. Even a comment in the github yaml worksforme.

Copy link
Member

@mikedep333 mikedep333 Sep 8, 2020

Choose a reason for hiding this comment

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

@@ -56,8 +56,9 @@ jobs:
python-version: ${{ matrix.python }}
- name: Install tox
run: |
sudo apt-get -yy install ansible
Copy link
Member

Choose a reason for hiding this comment

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

Why install it from apt rather than from pip? We are still reliant on pip to install the desired version.

Copy link
Member Author

Choose a reason for hiding this comment

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

The strangest thing: Installing ansible with pip breaks molecule in the tox venv, presumably by leaving incompatible versions of files in common directories. System (ubuntu packaged) ansible doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, yeah. Please add a comment why we're doing that though. So it isn't accidentally code cleaned up.

ansible.cfg Outdated Show resolved Hide resolved
roles/pulp_common/tasks/install_pip.yml Outdated Show resolved Hide resolved
roles/pulp_common/templates/settings.py.j2 Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
Comment on lines 29 to 41
test: $(MANIFEST)
tox -e $(TOX_ENV)
Copy link
Member

Choose a reason for hiding this comment

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

So in order to run molecule locally, I have to build and then run a test command?

Copy link
Member Author

Choose a reason for hiding this comment

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

You only need to run make test. The Makefile cares for the rest.

Copy link
Member

Choose a reason for hiding this comment

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

I think you mentioned this verbally. In order for me to run freeform molecule commands, I have to run make install && molecule .... Correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but beware, this is the wrong pr to discuss those things. This one ought to only introduce the repr filter, and shall be rebased once #387 is merged. I think the relevant parts here are already outdated.

@fao89
Copy link
Member

fao89 commented Aug 10, 2020

Also, this entire change (or 2 changes) needs a ticket or 2 in pulp.plan.io.

Even if it only affects developers, it is a very large change. Only small changes that are presumed to not be visible to users can get away without a ticket.

we have a .dev changelog entry: https://github.com/pulp/pulp_installer/blob/master/pyproject.toml#L37

@mdellweg mdellweg marked this pull request as draft August 12, 2020 11:32
@mdellweg mdellweg force-pushed the repr_filter branch 3 times, most recently from fed8fb1 to 6ec301e Compare September 25, 2020 09:20
@mdellweg mdellweg marked this pull request as ready for review October 26, 2020 09:54
galaxy.yml Show resolved Hide resolved
roles/pulp_common/tasks/repos.yml Show resolved Hide resolved
requirements.yml Show resolved Hide resolved
@@ -17,8 +17,8 @@
fail:
msg: "If you give one of pulp_webserver_tls_cert and pulp_webserver_tls_key, you must also give the other."
when:
- pulp_webserver_tls_cert | length or pulp_webserver_tls_key | length
- not pulp_webserver_tls_cert | length or not pulp_webserver_tls_key | length
- pulp_webserver_tls_cert is not mdellweg.filters.empty or pulp_webserver_tls_key is not mdellweg.filters.empty
Copy link
Member

Choose a reason for hiding this comment

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

@mikedep333
Copy link
Member

@mdellweg Do we still want to proceed with this?

@mdellweg
Copy link
Member Author

@mdellweg Do we still want to proceed with this?

Yes, i want to see this still. The way we generate python settings today is adventurous.

@mdellweg
Copy link
Member Author

@mikedep333 Finally came around to touch this up.
I released a new version of the filters collection (after fixing the releng there). And i added a changelog that will eventually point to this PR. We started to adopt this pattern in pulp-cli so we didn't need to duplicate every trivial PR into an issue for the sake of changelog. If you want, we can still file one. Also, is the misc category the right one?

@fao89 fao89 requested a review from mikedep333 October 21, 2021 13:03
@mdellweg
Copy link
Member Author

@mikedep333 ping

Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for this lovely refactoring!

@mdellweg mdellweg merged commit a542669 into pulp:main Feb 9, 2022
13 checks passed
@mdellweg mdellweg deleted the repr_filter branch February 9, 2022 19:49
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.

None yet

4 participants