ManageIQ / manageiq-providers-embedded_terraform

ManageIQ plugin for the Embedded Terraform provider.
Apache License 2.0
0 stars 11 forks source link

Sync Terraform Templates from git repo #5

Closed putmanoj closed 7 months ago

putmanoj commented 7 months ago

Sync Terraform Template

agrare commented 7 months ago

@putmanoj Update app/models/manageiq/providers/embedded_terraform/automation_ma…nager/configuration_script_source.rb isn't a very helpful commit message and having 6 commits with the same commit message is confusing. If you're fixing rubocop warnings or other small items I like to rebase and amend the commit which introduced the issue rather than adding a new commit.

agrare commented 7 months ago

TODO before merge:

jrafanie commented 7 months ago

@putmanoj I added some tests and made a few changes to the existing ones on my fork: https://github.com/ManageIQ/manageiq-providers-embedded_terraform/compare/master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

you can cherry-pick them or I can help you bring them over. Let me know what you think.

putmanoj commented 7 months ago

@putmanoj I added some tests and made a few changes to the existing ones on my fork: master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

you can cherry-pick them or I can help you bring them over. Let me know what you think.

Thanks, will cherry-pick.

Fryguy commented 7 months ago

@putmanoj Is this built on top of another PR? If so can you indicate that in the opening post? It's hard to follow with 24 commits.

agrare commented 7 months ago

@putmanoj please rebase on master rather than creating a new merge commit in this branch as it keeps the history cleaner

Merge branch 'git-sync-templates' of github.com:putmanoj/manageiq-providers-embedded_terraform into git-sync-templates d10963e

jrafanie commented 7 months ago

@putmanoj Ok, I think I resolved all of the comments with my 👍 in them. I think everything but provider.rb. I think that needs to be resolved. Let's remove that module and anything in there we don't need yet. We can add it later.

Please review my branch: at https://github.com/ManageIQ/manageiq-providers-embedded_terraform/compare/master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

I squashed our fixup commits. My final commits can be combined once you review them. Note, I really deleted a lot of tests because they still passed when I commented out most of the configuration source class so they're not really testing it.

I didn't want to push directly to your branch here but mine is correctly tracking upstream here so if you want to use my branch, we can push that.

putmanoj commented 7 months ago

Please review my branch: at master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

LGTM

jrafanie commented 7 months ago

ok, @putmanoj, I force pushed to your fork. I've addressed all concerns not yet addressed other than the questions about provider.rb including DefaultTerraformObjects from Jason and Adam. Actually, I think Adam said you might not even need provider.rb here. Other than that, we need to set the role properly for the queue items. I marked those commented tests as pending so we can properly set the role. I assume we want to use the new server role. I'm not sure what else is needed. Can you finish up those things and we can get ready for merge and then I can help get your other work rebased and tested? Thanks!

jrafanie commented 7 months ago

Note, https://github.com/ManageIQ/manageiq-providers-embedded_terraform/pull/9 was extracted from here

kbrock commented 7 months ago

I like the changes introduced by Adam and the other code looks good (from a person who doesn't use rugged enough)

LGTM