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

converted module to pdk #107 #120

Merged
merged 1 commit into from
Feb 22, 2018
Merged

converted module to pdk #107 #120

merged 1 commit into from
Feb 22, 2018

Conversation

enemarke
Copy link
Contributor

@enemarke enemarke commented Feb 9, 2018

Hi

I have converted the module to be pdk compliant and made a few minor refactors.

Some notes on what I have brooken and what is missing:

  • The kitchen setup was removed in the pdk convertion scripts and Im not familiar with kitchen and have not added kitchen config.
  • Readme has not been updated, since I would like some feedback on my work before updating it.

Cheers

@mcgege
Copy link
Member

mcgege commented Feb 12, 2018

Hi @enemarke , thanks a lot for working on this topic ... as I'm not the kitchen guru, maybe @artem-sidorenko can help us here?

I also would suggest to separate the functional optimization (/lib/*, /manifests/*) from the pdk / testing changes. Can you put the first block in a separate pr? I could test & merge it prior to this ...

Rakefile Outdated

task :default => [:run_all_linters, :spec]

# Changelog Generator
Copy link
Member

Choose a reason for hiding this comment

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

I definitely need this one for generating the Changelog

metadata.json Outdated
"version_requirement": ">= 4.0.0 < 6.0.0"
}
],
"template-url": "file:///opt/puppetlabs/pdk/share/cache/pdk-templates.git",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should change this to https://github.com/puppetlabs/pdk-templates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@enemarke
Copy link
Contributor Author

enemarke commented Feb 12, 2018

I have made a new commit that removed changes to /lib/, /manifest/ and some in /spec/. I hope this is the correct way for moving changes into a new pull request.

The rubocop test is now failing, due to the missing changes in /lib/* and /spec/.

Copy link
Member

@artem-sidorenko artem-sidorenko left a comment

Choose a reason for hiding this comment

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

@enemarke thanks for this PR! It makes definitely sense

I have two comments:

  • please do not remote test-kitchen - we need and use it for integration tests against linux-baseline
  • please avoid any big changes to Rakefile/.travis.yml if possible, almost everything there is used and makes sense the way it is. If you would like to introduce changes to the Rakefile/.travis.yml, please lets do it in a dedicated PR to allow easier review

.travis.yml Outdated
sudo: false
dist: trusty
Copy link
Member

Choose a reason for hiding this comment

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

this is not needed anymore, its a new default

@enemarke
Copy link
Contributor Author

@artem-sidorenko That makes sense. Should I close this pull request and open one or two new ones in order to get fewer cleaner commits or should I just make changes directly to this pull request?

@artem-sidorenko
Copy link
Member

@enemarke its up to you, you can also cleanup via rebase, squash the commits and force push your changes

@enemarke enemarke force-pushed the master branch 3 times, most recently from e50e357 to 5d1ef4f Compare February 14, 2018 13:58
@enemarke
Copy link
Contributor Author

enemarke commented Feb 14, 2018

I have rebased into fewer commits and removed changes to travis and test-kitchen.

@artem-sidorenko
Copy link
Member

I pushed it for integration tests: https://travis-ci.org/dev-sec/puppet-os-hardening/builds/341803747, it looks good to me. The failing tests are not fixed yet.

@enemarke could you maybe squash all other commits to one "pdk" commit?
@mcgege LGTM, your turn ;-)

@mcgege
Copy link
Member

mcgege commented Feb 17, 2018

@enemarke Also looks good to me ... could you please rebase your changes now on top of the master branch so that we could merge it?

@enemarke
Copy link
Contributor Author

Rebasing done.

@mcgege
Copy link
Member

mcgege commented Feb 20, 2018

@enemarke Retested it, looks good ... one final wish: Could you please rewrite your commit message, compact it and remove those comments, that have already been merged before?

@enemarke
Copy link
Contributor Author

@mcgege I have rewritten the commit message.

@mcgege
Copy link
Member

mcgege commented Feb 22, 2018

@enemarke Thanks a lot!

@mcgege mcgege merged commit 1fea019 into dev-sec:master Feb 22, 2018
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.

None yet

3 participants