-
Notifications
You must be signed in to change notification settings - Fork 4
Adapt for localstack-standalone-cli #49
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
Conversation
alexrashed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for jumping on this and updating this proactively! The pipeline is still red, and I think that's because with your changes you are dropping a hidden import that is necessary.
alexrashed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for fixing the imports!
| @@ -1,3 +1,3 @@ | |||
| pyinstaller | |||
| localstack==4.13.1 | |||
| localstack==4.13.2.dev36 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: How do you plan to get rid of this exact pin here to make sure that this is using the proper version after the next release of localstack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the release pipeline in pro to publish the standalone cli + make the pyinstaller build depend on it.
For this change we'd have two options: revert this to 4.13.1, but then the build will fail on main, or keep this change until the actual next localstack release comes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But wouldn't this break the pipeline here in main after the merge of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've edited the message before I got your response. Is this a response to what I planned before (reverting) or to what I've changed it to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's to the new one, how would it break on main if we keep the PR as-is? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by your message edits to be honest. So will this PR be merged at the time of the release? If so how does this play in the grand scheme of our release action?
If the PR is merged now, are you okay with breaking main?
Up to you, just want to make sure you are aware of the consequences / have a plan. 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge this now to match up with the latest dev release state of localstack. The next release will automatically change this to not be on the dev release anymore 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, perfect, thanks!
Motivation
The pyinstaller build needs some changes to work with the new standalone cli. The old build referenced
localstack_extandlocalstack.pro.coremodules that no longer exist in the newlocalstack_clinamespace.Changes
main.pyentry point to importlocalstack_cliMakefilepyinstaller arguments to uselocalstack_clilocalstack_extandlocalstack.pro.corehooks/hook-localstack_core.pywithhooks/hook-localstack.py