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

Closes #1124 - Add documentation for role developers #1186

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Mar 25, 2021

No description provided.

@wbclark
Copy link
Contributor Author

wbclark commented Mar 25, 2021

CC: @evgeni @ehelms @mdellweg @IncredibleRichie @bmarlow @willtome @hindman-redhat

I believe all of you have contributed, reviewed, or expressed interest in contributing to roles in this collection. Therefore your feedback on this documentation would be very valuable. What is it missing and how can it be improved to make this process better for future contributors?

Thanks,

@bmarlow
Copy link

bmarlow commented Mar 25, 2021

I think a simple contribution doc would be helpful by way of what kind of expectations the maintainers have for any submitted roles.

Specifically, calling out what expectations are for passing tests (90%+ all, etc), what kind of standards the maintainers would like (eg variable naming, documentation contents)

If there is a desire to have folks testing/building/developing against modules in the development branch, maybe docs on how to get them installed and available.


It is recommended to record the test fixtures against an uncofigured instance, so that the recorded fixtures will capture truly creating all necessary resources. You can ensure that your test instance is in this state before recording the fixtures by running `$ foreman-installer --reset-data`. This will drop and re-seed all databases and clear all synced content - do NOT run this on your production instance as you will lose ALL data!

Recording and running the tests requires an API doc fixture `tests/fixtures/apidoc/${name_of_role}_role.json`. This should typically just be a soft link to `katello.json` in the same directory. This fixture should be included with your pull request.
Copy link
Member

Choose a reason for hiding this comment

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

It's not always katello.


Recording and running the tests requires an API doc fixture `tests/fixtures/apidoc/${name_of_role}_role.json`. This should typically just be a soft link to `katello.json` in the same directory. This fixture should be included with your pull request.

Finally, to record the fixtures, use `make record_${name_of_role}_role`. This will run your test playbook against your test instance, and record all API requests and responses. When it is finished running, add and commit the fixtures at `tests/test_playbooks/fixtures/${name_of_role}_role-*.yml`.
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 basically the same for roles and modules. Should we rather link it and not duplicate the text?


## Changelogs

Don't forget to add a changelog fragment about your role! This is simple to do, just have a look at any previous example such as [this one](https://github.com/theforeman/foreman-ansible-modules/blob/2045c5edef5792d3235eb5c2b234ef08e3fa96b3/changelogs/fragments/1097-content-rhel-role.yml) to see how it should look.
Copy link
Member

Choose a reason for hiding this comment

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

Also this applies to any kind of plugin/role.
Should we move it to a contributing section?

Copy link
Member

Choose a reason for hiding this comment

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

Modules get automatic changelog entries right? Do the inventory plugins?

@@ -0,0 +1,32 @@
theforeman.foreman.your_new_role
Copy link
Member

Choose a reason for hiding this comment

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

I like that this strives for consistency.
But is it substantially different from what a-galaxy role init provides? Does it provide a README template?

Copy link
Member

Choose a reason for hiding this comment

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

You can provide your own skeleton to reference (https://docs.ansible.com/ansible/latest/galaxy/dev_guide.html#using-a-custom-role-skeleton) which we could do to include this README and at least a tasks/main.yml. As most roles do use the common role variables and have a common structure with the examples etc. I do find myself copying other roles READMEs and editing (and always missing updating something when I do).


Every role needs to have an integration test in the form of a playbook `tests/test_playbooks/${name_of_role}_role.yml`. You can look at the test playbook for an existing role in the collection to see an example of what this looks like.

CI does not test against a live instance; rather you are expected to record test fixtures from your own live instance and include them with the role. You can use [Forklift](https://github.com/theforeman/forklift) to quickly get up and running with a live virtual instance for testing. In some cases you may want to use a test instance with greater hardware resources available, if the role performs certain hardware intensive actions such as syncing large repositories.
Copy link
Member

Choose a reason for hiding this comment

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

For editablility and diff-ability i prefer a one sentence per line style.

@evgeni
Copy link
Member

evgeni commented Apr 27, 2021

need to be added to index.rst

@evgeni evgeni requested review from ehelms and mdellweg April 27, 2021 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants