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

Running of containerized commands refactored #2377

Merged

Conversation

k0taperk0t
Copy link
Contributor

@k0taperk0t k0taperk0t commented Oct 4, 2023

Change Overview

This PR removes code duplication and makes code of Makefile easier and clear. All commands for running containers moved to separated shell script.
These changes required for #2379. Without these changes we will have to make changes from #2379 twice - for shell and run targets of Makefile separately.

PR series:

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

@k0taperk0t
Copy link
Contributor Author

I agree to the DCO for all the commits in this PR.

@k0taperk0t k0taperk0t force-pushed the refactoring-of-running-containers branch from d07c62e to 925baf4 Compare October 4, 2023 14:40
@k0taperk0t k0taperk0t marked this pull request as ready for review October 4, 2023 14:41
@k0taperk0t k0taperk0t force-pushed the refactoring-of-running-containers branch 2 times, most recently from a9be933 to 5ed3d14 Compare October 5, 2023 08:41
usage
exit 1
esac

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


PWD="${PWD:-$(pwd)}"

# tag 0.1.0 is, 0.0.1 (latest) + gh + aws + helm binary
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment can be improved, I don't see 0.1.0 tag anywhere and which image tag are we talking about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commend just been moved from Makefile. Ok, I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@e-sumin
Copy link
Contributor

e-sumin commented Oct 6, 2023

LGTM but since I'm not an expert in shell scripting, I'd prefer to have second pair of eyes.

Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

LGTM

build/run_container.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Signed-off-by: Sergey Aksenov <sergey.aksenov@veeam.com>
Signed-off-by: Sergey Aksenov <sergey.aksenov@veeam.com>
Signed-off-by: Sergey Aksenov <sergey.aksenov@veeam.com>
@k0taperk0t k0taperk0t force-pushed the refactoring-of-running-containers branch from 81ea464 to cd1eb82 Compare October 6, 2023 13:56
@mergify mergify bot merged commit 4301258 into kanisterio:master Oct 6, 2023
14 checks passed
@k0taperk0t k0taperk0t deleted the refactoring-of-running-containers branch October 6, 2023 14:49
leuyentran pushed a commit that referenced this pull request Oct 18, 2023
* running of build & docs containers refactored

* A `run` rule fixed in Makefile

* build/run_container.sh + minor fixes in Makefile

Signed-off-by: Sergey Aksenov <sergey.aksenov@veeam.com>

* trivial review fixes

Signed-off-by: Sergey Aksenov <sergey.aksenov@veeam.com>

* review comments fixed

Signed-off-by: Sergey Aksenov <sergey.aksenov@veeam.com>

---------

Signed-off-by: Sergey Aksenov <sergey.aksenov@veeam.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants