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

Setup role for the Router Advertisement Daemon or radvd #490

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

cwilkers
Copy link
Contributor

@cwilkers cwilkers commented Nov 19, 2024

SUMMARY

Adds a role that installs the router advertisement daemon (radvd) that is a necessary component of dhcpv6 non SLAAC ipv6 deployments.

ISSUE TYPE
  • New or Enhanced Feature
Tests

If existing labs use SLAAC or no ipv6, it may not be advised to test this module in those labs until we can inspect their configuration.

TestBos2Sno: sno

@cwilkers cwilkers requested a review from a team as a code owner November 19, 2024 20:51
Copy link

@cwilkers cwilkers requested a review from a team as a code owner November 19, 2024 21:29
Copy link

Copy link
Contributor

@ramperher ramperher left a comment

Choose a reason for hiding this comment

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

Hi @cwilkers , thanks for your contribution! Some general advices below:

  • You need to review the logs from the ansible-lint check, that is currently failing. There you will find some regressions that you need to fix:
## Regressions from main branch:
-ERROR: roles/setup_radvd/tasks/main.yaml:: package-latest: Package installs should not use latest.
-ERROR: roles/setup_radvd/tasks/pre-requisites.yaml:: yaml[new-line-at-end-of-file]: No new line character at the end of file
  • For testing, ideally we test it with DCI, but in this case, since it's a new role that is not really related to DCI lifecycle, if you can provide some logs with a local execution you may run in your environment, it should be good.
    • In the meantime, if no DCI job is meant to be run, please add Test-Hints: no-check to your PR description, so that the DCI check will be omitted

roles/setup_radvd/README.md Outdated Show resolved Hide resolved
Copy link

@pierreblanc
Copy link
Contributor

recheck

Copy link

@pierreblanc
Copy link
Contributor

recheck

Copy link

Copy link

Copy link

Copy link
Contributor

@ramperher ramperher left a comment

Choose a reason for hiding this comment

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

LGTM, reviewed also some logs shared privately and the execution looks good.
@cwilkers , with regards to the PR, we will only need you to squash all your commits to have one single commit, then we can review what's going on with the sanity checks that are currently failing.

- Updated README with radvd role
- Uses indepent setup_radvd named variables
Copy link

@ramperher
Copy link
Contributor

Just recheck after merging #494, which includes the fix for the sanity checks.

@ramperher
Copy link
Contributor

@cwilkers , I think you will need to rebase your change to take the update from #494, then the sanity check issue should disappear

@fredericlepied
Copy link
Contributor

retest

@dcibot
Copy link
Collaborator

dcibot commented Nov 28, 2024

@fredericlepied fredericlepied added this pull request to the merge queue Nov 28, 2024
Merged via the queue into redhatci:main with commit f4d9cb5 Nov 28, 2024
7 checks passed
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.

6 participants