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

Refactor code #17

Merged
merged 31 commits into from
Jun 4, 2024
Merged

Refactor code #17

merged 31 commits into from
Jun 4, 2024

Conversation

lakkeger
Copy link
Contributor

@lakkeger lakkeger commented May 14, 2024

#Motivation
Our custom action needs some love in the form of refactor into a modular setup for better maintainability and separation of modules. The vision would be a dynamic setup and proxying through most options via the main action.

Changes

  • support of all currently available use cases via the main action
  • dynamic version calls for sub-modules
  • updated docs
  • updated test cases in ci

@lakkeger lakkeger force-pushed the refactor_default_action branch 24 times, most recently from a3538ea to 3faad6b Compare May 15, 2024 11:33
Copy link

github-actions bot commented May 15, 2024

⚡️ Running CI build with LocalStack ...

@lakkeger lakkeger force-pushed the refactor_default_action branch 5 times, most recently from c220895 to 31d526e Compare May 15, 2024 14:35
@lakkeger lakkeger force-pushed the refactor_default_action branch 2 times, most recently from b157dcb to bb050f6 Compare May 16, 2024 19:05
@lakkeger lakkeger force-pushed the refactor_default_action branch 2 times, most recently from 958c786 to 01d91d7 Compare May 17, 2024 08:06
@lakkeger lakkeger marked this pull request as ready for review May 23, 2024 07:34
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Kudos for tackling this refactoring @lakkeger - great to see we're dedicating some love to the setup-localstack action! 🚀

LGTM overall, added a couple of smaller comments/suggestions. Will defer the final approval to others - maybe @lukqw also has some additional feedback.. 👍

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
local/action.yml Outdated Show resolved Hide resolved
Copy link
Member

@lukqw lukqw left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for getting ahold of this change @lakkeger!

As teased in a couple of places I'd like to move away from the notion of preview in the future, and replace the term with ephemeral.
Might be something we can tackle before releasing 0.2.0, but lets talk offline.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ephemeral.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
local/action.yml Show resolved Hide resolved
preview/action.yml Show resolved Hide resolved
@lakkeger lakkeger merged commit 4b405a2 into main Jun 4, 2024
7 checks passed
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.

None yet

4 participants