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

Improve build script of examples #214

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

vcozzolino
Copy link
Contributor

@vcozzolino vcozzolino commented Oct 19, 2021

This new version of the example build script allows to selective build a type of example rather than all the Docker images. Moreover, it automatically pushes to a target repository.

@kubeedge-bot kubeedge-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 19, 2021
examples/build_image.sh Outdated Show resolved Hide resolved
@llhuii
Copy link

llhuii commented Oct 20, 2021

This new version of the example build script allows to selective build a type of example rather than all the Docker images. Moreover, it automatically pushes to a target repository.

Since its name is build_image.sh, so I don't think it is good idea to intruduce push function.

You can see push_image.sh, or add another script named build_push_image.sh

examples/build_image.sh Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
@vcozzolino
Copy link
Contributor Author

This new version of the example build script allows to selective build a type of example rather than all the Docker images. Moreover, it automatically pushes to a target repository.

Since its name is build_image.sh, so I don't think it is good idea to intruduce push function.

You can see push_image.sh, or add another script named build_push_image.sh

OK, I have removed the docker push command.

examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
EXAMPLE_REPO_PREFIX=${IMAGE_REPO}/sedna-example-

dockerfiles=(
dockerfiles_federated_learning=(
Copy link

Choose a reason for hiding this comment

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

Actually it can be improved by:

  • remove explict Dockerfiles here, using like ls *.Dockerfile, and then match by the subdirectories
  • Or move these Dockerfiles into their subdirectories

It can mitigate these explict Dockerfiles.

But because these dockerfiles are probably stable, it's optional, you can adopt if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could maybe apply this change later. It depends on the example folder structure and if we change it we might lose backward compatibility.

Copy link

@JimmyYang20 JimmyYang20 Oct 27, 2021

Choose a reason for hiding this comment

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

@vcozzolino there are two examples of federated learning. If the user only want to use the one, the other shouldn't be mirrored.

Copy link
Contributor Author

@vcozzolino vcozzolino Oct 27, 2021

Choose a reason for hiding this comment

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

I think this is more of a problem with the folder structure rather than the script. As @llhuii suggested, we use the directories name to build the associated Dockerfiles. If we want to do more specific pattern matching, we have a few options:

  1. Create two different directories for the two examples of federated learning such as federated_learning_mistnet and federated_learning_surface. It's anyway something you do in the nested directories. It's just a matter of moving up the splitting in the directory tree. This probably requires extra work to check that all dependencies and Dockerfiles are correctly updated.
  2. Move Dockerfiles into the subfolders. This breaks backward compatibility because the Dockerfiles use relative imports, so we will also have to change the content of each Dockerfile itself.
  3. Use Dockerfiles names to do the matching in the script rather than folder names. However, this might introduce ambiguity in the decision of which Dockerfiles to build.

Do you have suggestions?

Maybe option 3 is the least invasive.

Copy link

Choose a reason for hiding this comment

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

option 2 is higher cohesion, lower coupling. we can do it in future.

But for now the feature level is enough I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. So I guess we can go ahead with the current changes and plan for extra improvements in the future.

examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
@llhuii
Copy link

llhuii commented Oct 26, 2021

lgtm.
Some nits comment left. And I think you can squash these commits into one or two commits

@vcozzolino
Copy link
Contributor Author

lgtm. Some nits comment left. And I think you can squash these commits into one or two commits

All done.

@llhuii
Copy link

llhuii commented Oct 26, 2021

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021
@llhuii llhuii changed the title Improve build/push script of Sedna examples. Improve build script of examples Oct 26, 2021
examples/build_image.sh Outdated Show resolved Hide resolved
examples/build_image.sh Outdated Show resolved Hide resolved
@llhuii
Copy link

llhuii commented Oct 26, 2021

/lgtm cancel
a typo is need to be fixed

@kubeedge-bot kubeedge-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021
Signed-off-by: Vittorio Cozzolino <vittorio.cozzolino@huawei.com>
@llhuii
Copy link

llhuii commented Oct 27, 2021

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2021
@llhuii
Copy link

llhuii commented Oct 27, 2021

/cc @JimmyYang20 take a look

@kubeedge-bot
Copy link
Collaborator

@llhuii: GitHub didn't allow me to request PR reviews from the following users: take, a, look.

Note that only kubeedge members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @JimmyYang20 take a look

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@llhuii
Copy link

llhuii commented Oct 28, 2021

/approve

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: llhuii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2021
@kubeedge-bot kubeedge-bot merged commit de27dd7 into kubeedge:main Oct 28, 2021
@vcozzolino vcozzolino deleted the feature-build-script branch October 28, 2021 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants