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

Ansible Community Instruqt tracks #46

Closed
wants to merge 16 commits into from
Closed

Ansible Community Instruqt tracks #46

wants to merge 16 commits into from

Conversation

- apt-utils
- postgresql
- postgresql-common
- python3-psycopg2
Copy link

Choose a reason for hiding this comment

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

Note: This package set looks like it will only work on Debian-derivatives. Is that a problem or should it be called out as a prerequisite for this track?

@Andersson007
Copy link

Andersson007 commented Sep 9, 2021

Create your module scenario. (copied from direct chat in Slack)

  • The preparations with dev env feel complicated and unnecessary. Is it really needed for the purpose of this scenario? It can fear newbies. I'd recommend to get rid of that and use the simple approach that we use in the other recent scenarios.

  • I'd suggest to get rid of text highlighting because now it's hard to read as I mentioned in the previous review (or would highlight it using another style or in bold - we checked with Deric, it looks better imo)

  • I'd add everywhere something like To avoid indentation issues, type :set paste and press Enter, then press i

  • How to save changes and exit from vim.

  • (step 4) extends_documentation_fragment feels a bit unnecessary there.

  • I'd move all the links to external resources to the end of the course to not distract and interrupt users when they are in the middle of the process. E.g. To know in depth about the documentation block and its field, refer to the documentation here and other. They should stop learning or skip those links and i doubt that they will return to those pages after completing the course.

  • (step 5) vim ~/ansible_collections/example_ns/example_collection/plugins/modules should be changed to vim ~/ansible_collections/example_ns/example_collection/plugins/modules/my_test.py

  • (step 5) Note: If you're facing indentation issue while copying the code then revert the changes and run the following command: should be replaced with see the above.

  • (step 7) vim ~/ansible_collections/example_ns.example_collection/plugins/modules/my_test.py should be changed to vim ~/ansible_collections/example_ns/example_collection/plugins/modules/my_test.py

  • (step 7) Instead of To understand each and every line of the above module code, read the comments in the python code from the example here. (that can be moved to the end of the course), we can put short comments before each line what it does.

  • (last step) I would change ... executable binaries. to ... utilities.

@Andersson007
Copy link

Andersson007 commented Sep 9, 2021

Creating a collection scenario (copied from direct chat in Slack)

  • (step 1) To start with, install ansible-core package which also includes ansible-playbook and ansible-galaxy binaries. Maybe it would better to change binaries to utilities because IIRC it's just python text files.

  • (step 2) mkdir -p ~/collections/ansible_collections && cd ~/collections/ansible_collections
    I would also split the commands into separate steps as you did in the other scenarios OR
    would mention that (meaning) let's create and go to.. (same for other similar things)

  • General thing: maybe would be better to avoid highlighting things in text like module names, etc.? IMO it makes more difficult to see the boxes with code examples. And for me personally it's more difficult to focus on NOT highlighted text in same sentences. Or, if possible, we could highlight them different way, not so visible. Just a suggestion as everything else. (update: changing to bold should work)

  • General thing: how to save changes and exit vim:)

  • General thing: I'd also add :set paste + i as you did because that wrong indentation you mentioned happened to me this time... I've no idea why.

  • After running the playbook, i got (and the Check reported Correct):

TASK [Change TZ] ***************************************************************
fatal: [localhost]: FAILED! => {"changed": false, "cmd": "/usr/bin/timedatectl set-timezone Europe/Paris", "msg": "Failed to set time zone: Not authorized", "rc": 1, "stderr": "Failed to set time zone: Not authorized\n", "stderr_lines": ["Failed to set time zone: Not authorized"], "stdout": "", "stdout_lines": []}

@Andersson007
Copy link

Integration testing instruqt scenario. (copied from direct chat in Slack)

  • (step 2) Verify that you're on integration_example branch by running the below command. feels a bit extra as when we create the branch with that command, git reports that the branch has been changed

  • (step 3) There's highlighted Create /tests/integration/targets/setup_postgresql_db/tasks/main.yml and copy paste the below and then same with vim tests/integration/targets/setup_postgresql_db/tasks/main.yml . Can we make the sentence Create test/... a plain text? (update: or bold, it looks imo better)

  • I would also add note how to exit from vim:) This can definitely confuse people who use nano or something else:)

  • When I was doing step 4, i created only the dependency in postgresql_info target. I didn't notice that there are the tasks below (we need to scroll down to reach them) and press Check and it reported Success. Then I did the last step and i, of course, didn't get the expected result because the postgresql_info task hadn't been written. Would be nice to test and fix checks where needed and it should fail if users don't complete any steps. Can be really confused for unfamiliar folks.

@Andersson007
Copy link

Andersson007 commented Sep 9, 2021

Fix a bug scenario (copied from direct chat in Slack)

  • (step 2) vim ansible_collections/community/mysql/plugins/modules/mysql_user.py should
    be changed to vim plugins/modules/mysql_user.py

  • Once you've added the line, click the x on the top of the file name to edit the editor and save mysql_user.py i didn't see "X" anywhere:) Maybe just Save the changes and exit Vim :wq or something like that.

  • How to save and exit from Vim everywhere

  • I suggest leaving only ansible-test sanity plugins/modules/mysql_user.py --docker when we check the fix. I.e. To make it faster, we'll run sanity tests on the specific file ... or something.

  • If the test fails, reopen the file with the above command. -> If the test fails, reopen the file with the above command, fix it, and run the tests again.

  • step: Create a changelog fragment.

  1. we should do cd .. first
  2. I would add a period to the end of the fragment entry.
  3. The main issue i can see is that we don't add fragments for doc fixes:)
    Some collections require fragments for doc changes with trivial key so that ansibull-changelog will ignore it when building release notes. Maybe let's change minor_changes to trivial then?
  4. git commit -m "Fix mysql_user documentation" . i believe the period is not needed there
  • I removed the last commit (like i didn't do it at all) and pressed Check and it reported success. I didn't check other steps.

@Andersson007
Copy link

Andersson007 commented Sep 9, 2021

Testing a PR locally scenario

Generally looks good. To minor things

  • I'd change highlighting to bold or something else
  • How to save changes and exit from vim

Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
@grfeller
Copy link

I learned about these five tracks after signing up for the upcoming Ansible Contributor Summit - I noticed an issue that was not mentioned earlier in this thread.

Testing A Pull Request Locally track

Challenge: check-out-the-source-collection

Issue: pr_example branch does not exist in https://github.com/ansible-collections/ansible.posix.git

Relevant output:

Last login: Wed Sep 15 14:33:46 UTC 2021 on pts/0
[devops@shell ~]$ mkdir -p ~/ansible_collections/ansible/posix
[devops@shell ~]$ git clone https://github.com/ansible-collections/ansible.posix.git ~/ansible_collections/ansible/posix
Cloning into '/home/devops/ansible_collections/ansible/posix'...
remote: Enumerating objects: 1861, done.
remote: Counting objects: 100% (529/529), done.
remote: Compressing objects: 100% (257/257), done.
remote: Total 1861 (delta 379), reused 319 (delta 260), pack-reused 1332
Receiving objects: 100% (1861/1861), 492.35 KiB | 6.15 MiB/s, done.
Resolving deltas: 100% (1049/1049), done.
[devops@shell ~]$ cd ~/ansible_collections/ansible/posix
[devops@shell posix]$ cd ~/ansible_collections/ansible/posix
[devops@shell posix]$ git checkout pr_example
error: pathspec 'pr_example' did not match any file(s) known to git
[devops@shell posix]$ git branch -a
* main
  remotes/origin/HEAD -> origin/main
  remotes/origin/main

I noticed that the pr_example branch exists in the https://github.com/test-ansible/ansible.posix.git repository (test-ansible).

When I repeated the challenge using the test-ansible repository for the clone operation, I got the results that I expected (which may be different from the results intended...)

Change:
git clone https://github.com/test-ansible/ansible.posix.git ~/ansible_collections/ansible/posix

Result:
This challenge and the subsequent ones in this track worked for me.

@Ompragash
Copy link
Contributor Author

@grfeller the issue that you encountered is resolved and updated on the track as well:)

Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

Looks great. I know you've spent a lot of time on this.

@Ompragash
Copy link
Contributor Author

@gundalow Thanks for your review! Applied the suggestions to all the tracks.

This pull request was closed.
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

5 participants