[ONE-Dockerizing] Scripts that can run onecc using docker containers by holawan · Pull Request #9737 · Samsung/ONE

holawan · 2022-09-19T08:11:12Z

1st PR

This PR is a script that builds images and drives containers to run onecc with docker containers

ONE-DCO-1.0.-Signed-off-by: Dongwan Kim asdf134652@gmail.com

2nd PR

This PR is an improved script to reflect the review

How to get version
Functionalize duplicate use options
Check Dockerfile Existence
Run docker container

3rd PR

This PR has added error handling logic

4th PR

This PR was created to add README to "onecc-docker".

5th PR

This PR has added an API request excess exception handling code

parent issue: #9712 #9721
draft: #9722

/cc @Samsung/ootpg

This PR is a script that builds images and drives containers to run onecc with docker containers ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

compiler/onecc-docker/onecc-docker

mhs4670go · 2022-09-19T13:19:20Z

compiler/onecc-docker/onecc-docker

+    user_cmd = ''.join(sys.argv[1:])
+    run_cmd = ["docker", "run", "--rm", "--name", contianer_name, "-v", "${HOME}:${HOME}", "-e", "HOME=${HOME}", "-w", work_dir, image_name, user_cmd]
+    cmd = ' '.join(run_cmd)
+    subprocess.run(cmd,shell=True)


In this onecc-docker, there are three method call when the subprocess is invoked - subprocess.run, subprocess.call, Popen. Is there a reason to use three different functions?

How about make a method that runs a command?

def run_subprocess(cmd): ret = subprocess.run(cmd, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) # do something or return the results # error handling might be needed

holawan · 2022-09-19T13:52:35Z

@mhs4670go , @jyoungyun
There seems to be a lot of corrections in our code.
May I continue to talk in this PR and improve the code?

mhs4670go · 2022-09-19T14:19:19Z

May I continue to talk in this PR and improve the code?

Yes. Splitting the PR will split the information, which makes it difficult to find the history later.

compiler/onecc-docker/onecc-docker

jyoungyun · 2022-09-20T00:51:55Z

compiler/onecc-docker/onecc-docker

+import os
+import requests
+
+


Could you add usage for this script? The argparse library will help you to write user friendly interfaces.

You mean that make -h option on onecc-docker with argparse library?
I think that -h option is duplicated with onecc's -h option.
Should we make new flag for getting onecc's arguments?

I think onecc-docker can take own argument. For example, if the user want to pass a personal access token to the request headers, onecc-docker should take a token value from argument. In view of this case, this script should implement a separate argparser.

If onecc is already installed when running onecc-docker, shows onecc help message and if onecc is not installed, shows onecc-docker help message. What about this solution?

First of all, let's put the PR without considering -h option handling.

If onecc is already installed when running onecc-docker, shows onecc help message and if onecc is not installed, shows onecc-docker help message. What about this solution?

Yes, after finalizing existing developments, it would be a good idea to consider them!

First of all, let's put the PR without considering -h option handling.

Okay.

compiler/onecc-docker/onecc-docker

jyoungyun · 2022-09-20T01:10:14Z

compiler/onecc-docker/onecc-docker

+
+
+
+def main():


Unfortunately, I can't use the github rest api without authorization due to in-house network issue. So I suggest to support request header so that the users who need it can use it.

headers["Authorization"] = "token {}".format(/*token*/) response = requests.get("https://api.github.com/repos/Samsung/ONE/tags", headers=headers)

https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting
https://docs.github.com/en/rest/overview/resources-in-the-rest-api#oauth2-token-sent-in-a-header

jyoungyun · 2022-09-20T01:22:07Z

I think compiler/one-docker folder python files isn't checked with format checker.

I made #9742 PR.

@holawan Could you rebase this PR after merging #9742 PR. :)

compiler/onecc-docker/onecc-docker

This PR has added error handling logic ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

This PR was created to add README to "onecc-docker". ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

holawan · 2022-09-20T12:42:33Z

compiler/onecc-docker/onecc-docker

+        build_cmd = [
+            "docker", "build", "-t", image_name, "--build-arg", build_arg, script_path
+        ]
+        print('build Docker image ...')
+        _run(build_cmd)
+        print('Dockerfile successfully built.')


@jyoungyun , when the user does not have a docker image, the build time takes about 10 minutes. Is it necessary to print out the flow of the docker being built?

Yes, I think it is necessary.

This PR has added an API request excess exception handling code ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

mhs4670go · 2022-09-20T14:41:05Z

compiler/onecc-docker/README.md

@@ -0,0 +1,263 @@
+# onecc-docker
+
+`onecc-docker` supports `one-cmd`, which is currently only supported by ubuntu 18.04, in more OS.


`onecc-docker` -> _onecc-docker_

Suggested change

`onecc-docker` supports `one-cmd`, which is currently only supported by ubuntu 18.04, in more OS.

_onecc-docker_ supports `one-cmds`, which is currently only supported on ubuntu 18.04, in more OS.

Q) what is in more OS. ?

Q) what is in more OS. ?

More OSs can provide it.

only supported on ... in more OS.

I cann't catch the meaning of this sentense. can you elaborate a bit?

This means that onecc-docker supports running onecc in environments other than ubuntu:18.04.

How about

onecc-docker can expand one-cmds in more platforms than just Ubuntu 18.04/20.04.

or

We can use one-cmds in more platorms than just Ubuntu 18.04/20.04 through onecc-docker.

compiler/onecc-docker/README.md

mhs4670go · 2022-09-20T15:01:41Z

compiler/onecc-docker/onecc-docker

+import json
+import os
+import requests
+import argparse


This argparse has been added for https://github.com/Samsung/ONE/pull/9737/files#r974949214. Is it going to be used?

We haven't started the task on "argparse" yet.
I am trying to use it according to @jyoungyun's opinion.
I think I can give you a detailed answer after the meeting tomorrow.

Could you add token argument for in-house developers?

yes let's try that.

compiler/onecc-docker/README.md

jyoungyun · 2022-09-21T09:26:21Z

compiler/onecc-docker/onecc-docker

+import os
+import requests
+
+


If onecc is already installed when running onecc-docker, shows onecc help message and if onecc is not installed, shows onecc-docker help message. What about this solution?

First of all, let's put the PR without considering -h option handling.

jyoungyun · 2022-09-21T09:28:48Z

compiler/onecc-docker/onecc-docker

+import json
+import os
+import requests
+import argparse


Could you add token argument for in-house developers?

jyoungyun · 2022-09-21T09:29:53Z

compiler/onecc-docker/onecc-docker

+        build_cmd = [
+            "docker", "build", "-t", image_name, "--build-arg", build_arg, script_path
+        ]
+        print('build Docker image ...')
+        _run(build_cmd)
+        print('Dockerfile successfully built.')


Yes, I think it is necessary.

jyoungyun · 2022-09-21T09:41:30Z

compiler/onecc-docker/onecc-docker

+        response = requests.get(TAG_URL)
+        response.raise_for_status()
+    except requests.exceptions.RequestException as e:
+        raise SystemExit(e)


Could you user-friendly print an error message?

SystemExit('blah blah blah')

When error occured in requests for example "401 Client Error: Unauthorized for url: https://api.github.com/repos/Samsung/ONE/releases/latest" message is printed to bash (Git api sent this message from server)

Should I print more information except this?

@jyoungyun

Yes. I think the message is not friendly to the users. :)
You can see some error messages in https://github.com/Samsung/ONE/blob/master/compiler/one-cmds/utils.py file.

Thank you for advice! I'll check utils.py.

jyoungyun · 2022-09-21T09:53:02Z

compiler/onecc-docker/README.md

+
+
+
+## Examples


What about separating this content into a separate file like tutorial.md?

holawan · 2022-09-21T11:57:53Z

@jyoungyun , we will proceed with code refactoring by collecting your opinions. If our code has been fixed and you think it's okay to merge, can you please tell me?

seanshpark · 2022-09-22T00:55:34Z

compiler/onecc-docker/README.md

+## Description
+
+For now, `one-cmds` tools only support Ubuntu 18.04 and 20.04(not officially).
+So people in other environments can't use our tools unless they upgrade the OS (or install Ubuntu OS).


they upgrade the OS

Actually, users may use recent Ubutuntu 22.04. Thus it is likely they downgrade the OS

Actually, users may use recent Ubutuntu 22.04. Thus it is likely they downgrade the OS

How about changing it like this?

Therefore, it is difficult for people in different environments to use our tools without using ubuntu18:04.

compiler/onecc-docker/README.md

seanshpark · 2022-09-22T00:59:30Z

compiler/onecc-docker/README.md

+### Check Docker Image Presence and Build Image
+
+- To prevent repetitive image generation, we verify that there is an `onecc` image of the latest `one` release version.
+- If not, build the docker image using the docker file.


build -> rebuild ?

In my opinion, we build the docker image only once (unless the user intentionally deletes the image), what we repeat is considered to be the creation and deletion of the container.
@jyoungyun
What do you think?

build -> rebuild ?

@seanshpark I can't catch your intention. :( Could you explain more?
The docker image is built only once there is no image with the same name. So I think build is more suitable for this step...

Well, this maybe because I'm not a good Docker user/developer.
What I see is a Docker image. from the explanation, when the latest is changed, it sounds like Docker image is built again. So from the though of again, it looks like rebuild.
Technically it looks like build not rebuild so, we can go with build

compiler/onecc-docker/README.md

seanshpark · 2022-09-22T01:04:35Z

compiler/onecc-docker/README.md

+0 directories, 6 files
+```
+
+#### `-h` ,`--help`


maybe still WIP?

Do you mean help message only for onecc-docker? This is under discussion with @jyoungyun .

Let's discuss after the code supporting the input arguments is shared. :)

jyoungyun · 2022-09-22T02:20:38Z

@holawan

we will proceed with code refactoring by collecting your opinions. If our code has been fixed and you think it's okay to merge, can you please tell me?

Of course. If there is nothing more to modify, I will let you know. :)

This PR has added README fixes and Tutorial ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

This PR has added argmentParser to onecc-docker ONE-DCO-1.0.-Signed-off-by: Junsu Kim <xofkdqkqh@naver.com>

compiler/onecc-docker/README.md

[ONE-Dockerizing] Scripts that can run onec using docker containers

ca49738

This PR is a script that builds images and drives containers to run onecc with docker containers ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

holawan added the SSAFY label Sep 19, 2022

holawan requested a review from jyoungyun September 19, 2022 08:11

holawan assigned morumoru, Seunghui98 and holawan Sep 19, 2022

morumoru mentioned this pull request Sep 19, 2022

[ONE_Dockerizing] onecc-docker using recent version only #9738

Closed

holawan changed the title ~~[ONE-Dockerizing] Scripts that can run onec using docker containers~~ [ONE-Dockerizing] Scripts that can run onecc using docker containers Sep 19, 2022

This comment was marked as resolved.

Sign in to view

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 19, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 19, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

jyoungyun reviewed Sep 20, 2022

View reviewed changes

jyoungyun mentioned this pull request Sep 20, 2022

[infra] Add onecc-docker directory to code format items #9742

Merged

holawan marked this pull request as draft September 20, 2022 01:39

holawan added PR/NO MERGE Please don't merge. I'm still working on this :) DRAFT A draft issue or PR for sharing one's current working status and discussion. labels Sep 20, 2022

holawan force-pushed the onecc-docker branch from 03b8873 to 07f9592 Compare September 20, 2022 06:38

holawan commented Sep 20, 2022

View reviewed changes

compiler/onecc-docker/onecc-docker Show resolved Hide resolved

holawan added 3 commits September 20, 2022 21:28

[ONE-Dockerizing] Modify cmd execution, Error handling logic

1d9bb5d

This PR has added error handling logic ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

[ONE-Dockerizing] Add onecc-docker READEME

161efe1

This PR was created to add README to "onecc-docker". ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

Merge branch 'Samsung:master' into onecc-docker

25aa5cb

holawan commented Sep 20, 2022

View reviewed changes

[ONE-Dockerizing] Handling API Request Exceeded Execption

c9bb043

This PR has added an API request excess exception handling code ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

mhs4670go reviewed Sep 20, 2022

View reviewed changes

compiler/onecc-docker/README.md Outdated Show resolved Hide resolved

mhs4670go reviewed Sep 20, 2022

View reviewed changes

Seunghui98 mentioned this pull request Sep 21, 2022

[ONE-Dockerizing] debian package related to onecc-docker #9750

Closed

jyoungyun reviewed Sep 21, 2022

View reviewed changes

seanshpark reviewed Sep 22, 2022

View reviewed changes

compiler/onecc-docker/README.md Outdated Show resolved Hide resolved

seanshpark reviewed Sep 22, 2022

View reviewed changes

compiler/onecc-docker/README.md Outdated Show resolved Hide resolved

seanshpark reviewed Sep 22, 2022

View reviewed changes

compiler/onecc-docker/README.md Outdated Show resolved Hide resolved

seanshpark reviewed Sep 22, 2022

View reviewed changes

compiler/onecc-docker/README.md Outdated Show resolved Hide resolved

seanshpark reviewed Sep 22, 2022

View reviewed changes

compiler/onecc-docker/README.md Outdated Show resolved Hide resolved

seanshpark reviewed Sep 22, 2022

View reviewed changes

holawan and others added 2 commits September 22, 2022 17:39

[ONE-Dockerizing] Update READEME

d514a44

This PR has added README fixes and Tutorial ONE-DCO-1.0.-Signed-off-by: Dongwan Kim <asdf134652@gmail.com>

[ONE-Dockerizing] add argmentParser to onecc-docker

6a3a49e

This PR has added argmentParser to onecc-docker ONE-DCO-1.0.-Signed-off-by: Junsu Kim <xofkdqkqh@naver.com>

This was referenced Sep 22, 2022

[onecc-docker] Introduce onecc-docker #9771

Closed

[onecc-docker] Introduce onecc-docker #9772

Merged

jyoungyun reviewed Sep 23, 2022

View reviewed changes

compiler/onecc-docker/README.md Show resolved Hide resolved

This was referenced Sep 26, 2022

[SSAFY] ONE-Dockerizing (OOTPG) project review #9704

Closed

[ONE-Dockerizing] Introduce ONE Dockerizing #9712

Closed

holawan closed this Sep 26, 2022

		@@ -0,0 +1,263 @@
		# onecc-docker

		`onecc-docker` supports `one-cmd`, which is currently only supported by ubuntu 18.04, in more OS.

	`onecc-docker` supports `one-cmd`, which is currently only supported by ubuntu 18.04, in more OS.
	_onecc-docker_ supports `one-cmds`, which is currently only supported on ubuntu 18.04, in more OS.

		import os
		import requests

		import os
		import requests

Conversation

holawan commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

holawan commented Sep 19, 2022

Uh oh!

mhs4670go commented Sep 19, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morumoru Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyoungyun commented Sep 20, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhs4670go Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morumoru Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

holawan commented Sep 19, 2022 •

edited

Loading

morumoru Sep 20, 2022 •

edited

Loading

mhs4670go Sep 20, 2022 •

edited

Loading

morumoru Sep 22, 2022 •

edited

Loading