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 lan channel fact and user channel support #78

Merged
merged 9 commits into from
Sep 12, 2024

Conversation

nathanlcarlson
Copy link
Contributor

Based on #32 .

Intentionally named ::user parameter channel instead of lan_channel to be more general.

@jhoblitt
Copy link
Owner

@nathanlcarlson How would you feel about adding the channel as a structured fact. Something like ipmi.channel?

@nathanlcarlson
Copy link
Contributor Author

@jhoblitt Sure. Just this fact or should I create a structure for all impi* facts? The current flat facts could remain and I could add docs about them being deprecated?

@jhoblitt
Copy link
Owner

@jhoblitt Sure. Just this fact or should I create a structure for all impi* facts? The current flat facts could remain and I could add docs about them being deprecated?

I was only hoping to stop the growth of top level facts but if your willing to do the full conversion, that would be fantastic!

@nathanlcarlson
Copy link
Contributor Author

Kk, I made some standalone gathering and structuring of the facts. I'm not sure how REFERENCE.md gets updated or if there are other PDK items to take care of.

@nathanlcarlson
Copy link
Contributor Author

Ahh, I found the reference update command in the test results.

@nathanlcarlson
Copy link
Contributor Author

nathanlcarlson commented Sep 11, 2024

It looks like these remaining failures are related to CentOS package mirrors? Maybe partly related to the retirement of CentOS 7?

@nathanlcarlson
Copy link
Contributor Author

Those may be difficult to resolve. Any chance we can just ignore them?

@jhoblitt
Copy link
Owner

Those may be difficult to resolve. Any chance we can just ignore them?

I think we should remove centos completely from metadata.json in another PR labeled as a breaking-change. Do you want to take that on too? 🙏

@nathanlcarlson
Copy link
Contributor Author

I did my best to track down this issue and opened this voxpupuli/beaker-docker#144 and am trying the advice there.

@jhoblitt
Copy link
Owner

It's the plumbing in those module that needs to be updated. I'll run msync on it shortly.

@jhoblitt
Copy link
Owner

@nathanlcarlson CI is passing on master now. Could you try a rebase?

lib/facter/ipmi.rb Outdated Show resolved Hide resolved
Copy link
Owner

@jhoblitt jhoblitt left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for doing the fact conversion!

@jhoblitt jhoblitt merged commit 0af7e67 into jhoblitt:master Sep 12, 2024
18 checks passed
This was referenced Sep 12, 2024
@jhoblitt
Copy link
Owner

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.

2 participants