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

Shell.pm: add iLO support #1468

Closed
wants to merge 1 commit into from
Closed

Conversation

alip
Copy link
Contributor

@alip alip commented Feb 12, 2021

This PR is an attempt to fix # by .

Checklist

  • based on top of latest source code
  • changelog entry included
  • tests pass on Travis CI
  • git history is clean
  • git commit messages are well-written

@alip alip mentioned this pull request Feb 12, 2021
Copy link
Member

@ferki ferki left a comment

Choose a reason for hiding this comment

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

As discussed on IRC, we don't have access to iLO interfaces on the upstream side, so I can't responsibly commit to maintaining support long-term.

Therefore this module would be best to be maintained outside core in its own repo. Ideally it could also be published to CPAN as a standard Perl distribution. Feel free to reach out to us if there are any questions around that.

The mapping part between the shell name and the corresponding module can (and should) go into core for now. If there's a more generic solution to handle that mapping, I believe that would be highly interesting as a separate issue/PR.

@alip alip force-pushed the upstream-ilo-support branch from 0ed1472 to ad6ff61 Compare February 23, 2021 12:28
@alip
Copy link
Contributor Author

alip commented Feb 23, 2021

Thank you for the feedback. I have updated this PR to only include the mapping and created a git repository to host the module here where I fixed the copyright and formatted the code with perltidy. @ferki, would you please be so kind to take a quick look at the aforementioned repository before I make a release to see if it looks good? Thank you.

Copy link
Member

@ferki ferki left a comment

Choose a reason for hiding this comment

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

Hmm, since there seems to be no explicit shell detection tests implemented yet, rsync tests are failing now (luckily! :) )

After a quick look, the shell detection is triggered around these lines in Rex::Interface::Exec::Base, and the problem looks like to be about trying to load a missing module.

Ideally, I would prefer this to be solved by trying to only load the shell modules that are known to be present.

A quick and dirty alternative could be to add error handling (and debugging messages) for the "can't load this shell module, skipping" case. But that feels like handling the error after it happened instead of preventing it from happening, and I would only depend on this if we can't do better.

What do you think, should we take this topic into a separate issue/PR (which would have been the normal workflow anyway :) ) ?

EDIT: basically the same goes for the other repo/PR #1469 too.

@ferki
Copy link
Member

ferki commented Feb 24, 2021

@ferki, would you please be so kind to take a quick look at the aforementioned repository before I make a release to see if it looks good? Thank you.

Based on a quick look, my first thoughts dumped:

  • through your previous experience with the code, I trust the logic to be working
  • I also trust he copyright/license info to be correct now (I'm not a lawyer, though, and also not in the position to decide/judge)
  • I like the authoring and workflow bits in dist.ini (rules for code style, code quality, versioning, generating files, etc.) since it seems to copy some of my previous projects, but I would advise to double check if it's matching how you want to maintain/publish it
  • there's no real tests, so prove and dzil test --all doesn't seem to be happy yet

Basically the same goes for the other repo/PR #1469 too.

@ferki
Copy link
Member

ferki commented Mar 13, 2021

As briefly discussed via IRC earlier, I'm closing this one in favor of discussing a more complete solution via #1481.

@ferki ferki closed this Mar 13, 2021
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.

2 participants