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 plugin foreman_column_view #601

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

dgoetz
Copy link
Member

@dgoetz dgoetz commented Oct 18, 2017

This PR adds support for the plugin foreman_column_view.

In my tests it worked fine, but there are some points I am not sure about:

  • I used Puppet 4+ Datatypes in an excessive way for validation as some other code is already doing this but only in a much more simple way
  • I fear using a hash of hashes will not work with kafo, only with Puppet
  • I only added the default tests and one positive test for the configuration file, no negative one to fail if configuration is wrong
  • Changes of the plugin's configuration require a restart of Foreman which I did not address

Please let me know if something requires a change in the PR.

manifests/plugin/column_view.pp Outdated Show resolved Hide resolved
templates/foreman_column_view.yaml.erb Outdated Show resolved Hide resolved
manifests/plugin/column_view.pp Outdated Show resolved Hide resolved
spec/classes/plugin/column_view_spec.rb Show resolved Hide resolved
@dgoetz dgoetz force-pushed the feature/column_view branch 2 times, most recently from 52785ef to 0c4472f Compare October 18, 2017 17:14
@dgoetz
Copy link
Member Author

dgoetz commented Oct 18, 2017

Thanks for the review!
I changed the template to work with columns undefined so I do no longer require the if clause and can use the parameter config.
I moved the data type to a custom data type and added the check.
I also addressed the lint error from the checks, but I have no idea why the test failed now with a timeout.

manifests/plugin/column_view.pp Outdated Show resolved Hide resolved
types/column_view.pp Outdated Show resolved Hide resolved
spec/classes/plugin/column_view_spec.rb Show resolved Hide resolved
manifests/plugin/column_view.pp Outdated Show resolved Hide resolved
@dgoetz dgoetz force-pushed the feature/column_view branch 2 times, most recently from ac36f83 to d318401 Compare October 18, 2017 19:15
@dgoetz
Copy link
Member Author

dgoetz commented Oct 18, 2017

Thanks again!
Removed the undef and optional as it worked fine without it. Managing the file is intended as I checked that only the header is still a valid configuration for the plugin.
Removed the unused variable.
And moved the datatype from a hash with a minimum and maximum numbers of key value pairs (the 3,9) to a defined Struct. Added additional checks while doing so as it took me a while to understand how this should work. But now the checks all fail, while local check run successful (using the puppet 5.3.2 gem). Any idea?

@ekohl
Copy link
Member

ekohl commented Oct 19, 2017

It does show up with rake lint but I haven't been able to pin it down where exactly. I'll need to dive into it.

@ekohl
Copy link
Member

ekohl commented Oct 19, 2017

Looking into it comes from https://github.com/theforeman/kafo/blob/master/lib/kafo/data_type.rb but there is a Struct class. Somehow it's parsing Struct[ instead so there might be an issue with the regex.

@ekohl
Copy link
Member

ekohl commented Oct 19, 2017

I opened https://projects.theforeman.org/issues/21398. Not sure how to easily proceed with this but maybe we can ignore the puppet class that includes it from linting or that particular test.

@dgoetz
Copy link
Member Author

dgoetz commented Oct 20, 2017

Thanks for all the effort.

@ekohl
Copy link
Member

ekohl commented Oct 30, 2017

I wonder if we should accept the PR now with a simple type check while we work on the kafo issue.

@dgoetz
Copy link
Member Author

dgoetz commented Oct 30, 2017

I do not see a need to rush. Column_view is easily installed and configured manually, I just recognized it is the only one missing in the module and installer of those I commonly use. Also it will not make it into 1.16 if merged now, or will it? Furthermore kafo can not handle arrays of hashes, so it will only handle installation and not configuration.

@ekohl
Copy link
Member

ekohl commented Jun 2, 2018

So the actual problem is https://github.com/theforeman/kafo/blob/93aaa70eff832fc6ccfbcbbf672f5cd775007420/lib/kafo/data_type_parser.rb#L5 which can't parse multi line types. A multi-line regex sounds like a bad idea so we probably need to find a better way. We can probably extract them using puppet-strings and cache them like we do with others.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

This workaround has been on my mind for a while, but I always forgot to comment it here

manifests/plugin/column_view.pp Outdated Show resolved Hide resolved
spec/classes/plugin/column_view_spec.rb Outdated Show resolved Hide resolved
manifests/plugin/column_view.pp Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Let's see if Travis agrees :)

@dgoetz
Copy link
Member Author

dgoetz commented Aug 3, 2020

I had to adjust the checks because it needed a different string to match but Travis is green now! 😃

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl ekohl merged commit d755349 into theforeman:master Aug 3, 2020
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.

3 participants