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

Switch devel env to use pulplift - Issue 4234 #61

Merged
merged 2 commits into from Jan 14, 2019

Conversation

CodeHeeler
Copy link
Contributor

No description provided.

Added pulp3-devel role to the ansible-pulp3 repo

re #4234
https://pulp.plan.io/issues/4234
@@ -99,5 +99,12 @@
with_dict: '{{ pulp_install_plugins }}'
when: pulp_install_plugins[item.key].source_dir is defined

- name: Install requirements for building docs
Copy link
Member

Choose a reason for hiding this comment

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

This is supposed to go into the pulp3-devel role. Otherwise these deps are going to be installed for users that are just installing pulp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, good catch.

@CodeHeeler CodeHeeler force-pushed the issue-4234 branch 2 times, most recently from 1255a8a to 5dcc273 Compare January 10, 2019 23:53
@asmacdo asmacdo changed the title Issue 4234 Switch devel env to use pulplift - Issue 4234 Jan 14, 2019
pulp_source_dir: "/home/vagrant/devel/pulp"
pulp_plugin_source_dir: "/home/vagrant/devel/pulpcore-plugin"
pulp_secret_key: "unsafe_default"
developer_user: "vagrant"
Copy link
Contributor

Choose a reason for hiding this comment

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

Source install will definitely be used by non-vagrant users. IMO, anything vagrant specific should should be in the Vagrantfile.source.example (maybe change that to Vagnrantfile.dev.example)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nevermind. I already screwed this up by including the vagrant-pretask. We'll need to revisit which playbooks/Vagrantfiles are provided by us, and specify what they are intended for (ie, as an example to be changed, or as a playbook for developer environments, etc). However, since I already blurred the line, how you are doing it makes sense for now.

- dnf-utils
- dstat
- fd-find
- fedora-easy-karma
Copy link
Contributor

@asmacdo asmacdo Jan 14, 2019

Choose a reason for hiding this comment

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

Can we remove this package and some of the others? I don't many of these tools, can we reduce this to the minimum of tools that are used by the aliases for now, and any tools that others want can be an additive change later?

- httpie
- iotop
- jnettop
- jq
Copy link
Contributor

Choose a reason for hiding this comment

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

leave this, its in our docs

- fedora-easy-karma
- fpaste
- fzf
- git
Copy link
Contributor

Choose a reason for hiding this comment

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

I think git is already installed

- fzf
- git
- htop
- httpie
Copy link
Contributor

Choose a reason for hiding this comment

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

leave this, its in our docs

- jq
- ncdu
- python-django-bash-completion
- python-ipdb
Copy link
Contributor

Choose a reason for hiding this comment

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

I use this all the time, but it is not in our docs. For packages in this category, IMO, devs should install on their own. Its really easy to make a personal role and add it to a Vagrantfile or whatever. This way, we don't waste time waiting on the installation of tools that other people sometimes use.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, this should be removed. If we install this, it should be pip installed, not dnf.

- ncdu
- python-django-bash-completion
- python-ipdb
- python-rpdb
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ipdb

Copy link
Contributor

Choose a reason for hiding this comment

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

on second thought, i think take this out. many on the team use pycharm's remote debugger, and i almost exclusively use ipdb while running processes in the foreground. if this annoys people, they can always add it back.

if this goes, telnet should also go.

- redhat-lsb-core
- ripgrep
- screen
- telnet
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have an alias that used this, but I don't see it "pdebug". This is only necessary if we have rpdb installed. I say,we remove this.

- telnet
- tmux
- tree
- wget
Copy link
Contributor

Choose a reason for hiding this comment

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

not in the docs, but please leave it anyway. so basic.

- tmux
- tree
- wget
- vim-enhanced
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 leave it in +0 take it out. vi is already installed, and this package can be somewhat slow to install and has caused problems in the past.

Copy link
Member

Choose a reason for hiding this comment

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

Since the source is mounted into the Vagrant box, everyone can use their own favourite editor (with their systemwide configuration). I'd say we don't need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reducing to a minimal set of installed packages has been moved to 4322:
https://pulp.plan.io/issues/4322
And I have a PR up here where we can continue this discussion of packages:
#63

- python-ipdb
- python-rpdb
- python3-setuptools
- python3-virtualenvwrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

this is used by our "workon pulp" syntax, please leave in.

- dstat
- fd-find
- fedora-easy-karma
- fpaste
Copy link
Contributor

Choose a reason for hiding this comment

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

i think lets get rid of this

}
_pstatus_help="Report the status of all pulp-related services"

preset() {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets get rid of this.

pclean() {
workon pulp
pulp-manager reset_db --noinput
pulp-manager migrate auth --noinput
Copy link
Contributor

Choose a reason for hiding this comment

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

this line can be removed now

}
_phelp_help="Print this help"

alias phttp="http"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a pretty useless alias, given that its longer than the thing it aliases :)

I think it can be changed to http --auth admin:admin which would be more useful, since we set the password. Either remove or change.

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, we set the password in .netrc so +1 for removal.

pulp_source_dir: "/home/vagrant/devel/pulp"
pulp_plugin_source_dir: "/home/vagrant/devel/pulpcore-plugin"
pulp_secret_key: "unsafe_default"
developer_user: "vagrant"
pulp_create_user: false
developer_user_home: "/home/vagrant"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are starting to get a longish list, could you alphabetize the vars?

@@ -20,3 +25,4 @@
- pulp3-workers
- pulp3-resource-manager
- pulp3-webserver
- pulp3-devel
Copy link
Contributor

Choose a reason for hiding this comment

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

Does pulp3-devel work without the previous roles? If not, perhaps we should remove them and put them in the pulp3-devel roles meta dependencies.

@CodeHeeler CodeHeeler force-pushed the issue-4234 branch 2 times, most recently from c7600e1 to 857a3ac Compare January 14, 2019 20:43
Switching devel environment to use pulplift first focused on hooking up,
now focusing on cleaning up unused changes along the way or unwanted items.
Added doc building requirements

re #4234
https://pulp.plan.io/issues/4234
Copy link
Member

@dkliban dkliban left a comment

Choose a reason for hiding this comment

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

I just tested this and it works as expected. Thank you @CodeHeeler for this great contribution!

The suggested changes from @asmacdo are being tracked in https://pulp.plan.io/issues/4322

@dkliban dkliban merged commit ddfb88d into pulp:master Jan 14, 2019
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