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

Move up_args after the up command. #988

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ncstate-daniel
Copy link

@ncstate-daniel ncstate-daniel commented Aug 7, 2024

Summary

Fixes issue #984 where up_args are being placed before up, and need to go after.

Additional Context

Since 'compose' became an argument, the insert statement just need to be incremented by one to represent the new position. The other insert seems fine where it is.

Related Issues (if any)

#984

  • Manually verified. (For example puppet apply)

@ncstate-daniel ncstate-daniel requested a review from a team as a code owner August 7, 2024 16:53
@CLAassistant
Copy link

CLAassistant commented Aug 7, 2024

CLA assistant check
All committers have signed the CLA.

@@ -74,7 +74,7 @@ def get_image(service_name, compose_services)

def create
Puppet.info("Running compose project #{name}")
args = ['compose', compose_files, '-p', name, 'up', '-d', '--remove-orphans'].insert(3, resource[:options]).insert(5, resource[:up_args]).compact
args = ['compose', compose_files, '-p', name, 'up', '-d', '--remove-orphans'].insert(3, resource[:options]).insert(6, resource[:up_args]).compact
Copy link
Collaborator

Choose a reason for hiding this comment

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

The usage of #insert make it a bit hard to follow. I think we could benefit from re-writing this as something like:

args = ['compose', compose_files, '-p', name, resource[:options], 'up', resource[:up_args], '-d', '--remove-orphans'].flatten.compact

(assuming the index of the first insert is wrong because I don't think it is correct to insert resource[:options] between -p and name).

Also consistently using long options helps for readability: -p => --project-name, -d => --detach

Copy link
Author

Choose a reason for hiding this comment

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

If y'all want to do it a different way that's certainly cool with me. =) I needed this fixed asap and submitted my fix without modifying the way things are done too much.

Copy link
Author

Choose a reason for hiding this comment

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

(fwiw i agree -- i don't entirely understand why it was done the way it was)

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.

3 participants