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 support for RedHat distros (and Nobara) #157

Closed
wants to merge 18 commits into from

Conversation

Commandcracker
Copy link

It might even be a good idea to set firefox_package_name: firefox as a default for any other distros than Debian because most use Firefox.

@Commandcracker
Copy link
Author

The error message for unsupported distros:

TASK [staticdev.firefox : Install Firefox] **************************************************************************************************************************
fatal: [localhost]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'firefox_package_name' is undefined. 'firefox_package_name' is undefined\n\nThe error appears to be in '/home/red/.ansible/roles/staticdev.firefox/tasks/main.yml': line 34, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: Install Firefox\n  ^ here\n"}

The error message might confuse ansible noobs like me but I don't know if there is a good way of changing it.

Copy link
Owner

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

@Commandcracker thanks for the contribution. In order to add support to new distros we also need to add tests, metadata and on readme that it is supported. Could you please also add them?

@Commandcracker
Copy link
Author

Commandcracker commented Apr 23, 2023

@Commandcracker thanks for the contribution. In order to add support to new distros we also need to add tests, metadata and on readme that it is supported. Could you please also add them?

I have only added "RedHat" platforms that have firefox as their firefox package.
(RHEL has no official firefox package. There is only one with noscript for Enterprise Linux 7)
I'll take a look into testing with molecule!

meta/main.yml Outdated

- name: Fedora
versions:
- "39" # rawhide
Copy link
Owner

Choose a reason for hiding this comment

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

We should only add here what we are adding to tests. In this case EL and Nobara.

Good example of a role that supports more distros:
https://github.com/geerlingguy/ansible-role-docker/blob/master/meta/main.yml#L12

https://github.com/geerlingguy/ansible-role-docker/blob/master/.github/workflows/ci.yml#L44

Copy link
Author

Choose a reason for hiding this comment

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

EL is not supported as it dos not have a Firefox package, this is why I did it how I did it. And everything is based on fedora:

flowchart LR
    Fedora --> CentOS
    Fedora --> RHEL
    CentOS --> CentOSStream
    RHEL --> AlmaLinux
    RHEL --> RockyLinux
    RHEL --> OracleLinux
    Fedora --> Nobara
Loading

That's why i only did tests for fedora, but I see a reason for adding testing for the other distros. (like RHEL not having a Firefox package)
Adding tests for Nobara might be a bit hard as there are no docker images for it.

Copy link
Owner

Choose a reason for hiding this comment

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

It is ok like this, we just don't need that many old versions.

@Commandcracker
Copy link
Author

I wasn't able to run the workflow because act wasn't able to install podman in a docker container :D but I double checked everything.

# - ubuntu2204
- ubuntu2004
- ubuntu2004 # Focal
- ubuntu1804 # Bionic
Copy link
Owner

Choose a reason for hiding this comment

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

I have no intention of supporting Ubuntu versions that are EOL (older than 2004). Same for all those version of Fedora besides 37 and 38. And CentOS 8. There are limits of resources a repo can use on GA and I don't plan to go close to them nor wait hours to merge simple changes in the repo, there is no benefit for this much overhead. This is also a practice you can see in almost all Ansible repos.

Copy link
Author

Choose a reason for hiding this comment

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

@Commandcracker there are some last linting errors.. also, I think you will need to revert Ubuntu 22.04 because of a bug that I had commented in the issue you removed.

the comment is still there, ill investigate it.

@staticdev
Copy link
Owner

@Commandcracker there are some last linting errors.. also, I think you will need to revert Ubuntu 22.04 because of a bug that I had commented in the issue you removed.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@Commandcracker
Copy link
Author

yamllint AnsibleLintRule's seam to be outdated
I used get-galaxy-platforms to see if they are valid and 38, 37 are in there.

@staticdev
Copy link
Owner

staticdev commented Apr 24, 2023

yamllint AnsibleLintRule's seam to be outdated I used get-galaxy-platforms to see if they are valid and 38, 37 are in there.

I doubt the problem is on ansible-lint.. the link you sent me was updated 4 years ago. I would suggest you replace then 37 and 38 with all as in the example you sent me. So we don't need this disable lint.

@Commandcracker
Copy link
Author

Commandcracker commented Apr 24, 2023

yamllint AnsibleLintRule's seam to be outdated I used get-galaxy-platforms to see if they are valid and 38, 37 are in there.

I doubt the problem is on ansible-lint.. the link you sent me was updated 4 years ago. I would suggest you replace then 37 and 38 with all as in the example you sent me. So we don't need this disable lint.

You need to run get-galaxy-platforms.py. It fetches the latest platforms from ansible-galaxy and after you did that, you can see that 38 and 37 are in there!

I only disabled the lint in the section for Fedora versions

@Commandcracker
Copy link
Author

@staticdev staticdev added the enhancement New feature or request label Jun 17, 2023
Copy link
Owner

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

@Commandcracker seems like Ubuntu 22.04 requires extra effort to make it work because of default snap installation (I have a parallel PR for that). I also removed Debian 10 and added Debian 12. I would prefer if you can remove changes related to Ubuntu and Debian from your PR and keep only RedHat/Nobara related. You will need to rebase with main branch.

@staticdev
Copy link
Owner

staticdev commented Jun 18, 2023

@Commandcracker I ended up investing some time and incorporated your changes through:

So you don't need any changes and should work for you.

I released 2.0.0, you are in the credits, thanks a lot https://github.com/staticdev/ansible-role-firefox/releases/tag/2.0.0

@staticdev staticdev closed this Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants