build and push docker on release #213

mpnowacki-reef · 2023-10-21T14:50:10Z

No description provided.

mjurbanski-reef

nice work

mjurbanski-reef · 2023-10-27T13:51:25Z

pyproject.toml

-# Including project:version breaks `version` number generation under gha CD
-# [project]
-# requires-python = ">=3.7"
-# name = "b2"
-# version = "0.0.0"  # this is wrong, but setuptools>61 insists its here
+[project]
+requires-python = ">=3.7"
+name = "b2"
+dynamic = ["version"]


mjurbanski-reef · 2023-10-27T13:56:27Z

test/integration/conftest.py

+    tmp_readme = pathlib.Path(f'{TEMPDIR}/README.md')
+    if not tmp_readme.exists():
+        tmp_readme.write_text(
+            (pathlib.Path(__file__).parent.parent.parent / 'README.md').read_text()


ROOT_PATH = pathlib.Path(__file__).parent.parent.parent on top of this file would be improve readability

mjurbanski-reef · 2023-10-27T13:59:45Z

test/integration/test_b2_command_line.py

@@ -61,7 +64,7 @@ def get_bucketinfo() -> Tuple[str, str]:

 def test_download(b2_tool, bucket_name):

-    file_to_upload = 'README.md'
+    file_to_upload = f'{TEMPDIR}/README.md'


would be better as sample_file fixture/constant so we can change it in single place next time

mjurbanski-reef · 2023-10-27T14:02:33Z

test/integration/conftest.py

+def copy_readme_for_tests():
+    """Copy the README.md file to /tmp so that docker tests can access it"""
+    tmp_readme = pathlib.Path(f'{TEMPDIR}/README.md')
+    if not tmp_readme.exists():


I wanted to comment how this won't always update the readme... but I see now this doesn't matter at all as we do not care about readme contents as at all as we really ne ANY sample file;

which brings again this would better be served by sample_file fixture that would return sample file path - if that file happens to be a readme copied from somewhere wouldn't matter then.

mjurbanski-reef · 2023-10-27T14:11:00Z

test/integration/test_b2_command_line.py

+    if is_running_on_docker:
+        pytest.skip('Not supported on Docker')


To the best of my knowledge this can’t work. Running it yields strange errors about file descriptors not existing on the container os

oh, right. thanks

mjurbanski-reef · 2023-10-27T14:13:05Z

test/integration/test_b2_command_line.py

+    shutil.copyfile(
+        pathlib.Path(__file__).parent.parent.parent / 'CHANGELOG.md', f'{TEMPDIR}/CHANGELOG.md'
+    )


seems like we have two different places in which we create these sample files

good catch!

mjurbanski-reef · 2023-10-27T14:18:27Z

test/integration/helpers.py

+        :param cmd: a command to run
+        :param args: command's arguments
+        :param additional_env: environment variables to pass to the command, overwriting parent process ones
+        :param env_file_cmd_placeholder: If specified, all occurrences of this string in `args` will be substituted with a


no such argument

mjurbanski-reef · 2023-10-27T14:36:31Z

test/integration/test_b2_command_line.py

+    sse_c_key_id = 'user-generated-key-id \nąóźćż\nœøΩ≈ç\nßäöü'
+    if is_running_on_docker:
+        # TODO: fix this once we figure out how to pass env vars with \n in them to docker, docker-compose should work
+        sse_c_key_id = sse_c_key_id.replace('\n', '')


export TEST_VAR=$(echo "a\nb") docker run --rm -e TEST_VAR ubuntu env

this will require dropping env-file support and replacing it with -e ENVVAR listing, but what can you do.
If you don't like idea of putting docker-specific solutions into pytest (which we already do with skips), we can potentially write a docker_env_run.sh that would prepare -e $(env | awk -F "=" '{printf "-e %s=%s ", $1, $2}' inside of it and pytest would call it thru --sut as any other command

honestly I'm not sure this is worth, do you think so? It's a rare corner case, that probably doesn't require testing anyway

I'm just scared about someone loosing a ton of time on debugging this when they actually need it.

I guess solution in the middle would be to emit logger warning in https://github.com/reef-technologies/B2_Command_Line_Tool/pull/213/files#diff-0457e1c77e315509ddcb03e30c7d249298c33f012541c7e3654b6728e7cf56b5R424 IF any of the vars contains newlines. That way when adding a new test it blows up the warning will be visible in pytest output.

mjurbanski-reef · 2023-10-27T14:41:21Z

test/integration/test_b2_command_line.py

-        log = logfile.read()
-        assert re.search(log_file_regex, log), log
-    os.remove('b2_cli.log')
+    if not is_running_on_docker:  # It's difficult to read the log in docker in CI


looking at this and README problem, I wonder if mounting local folder as workdir in docker would kill both birds with one stone
using -u $(id -u):$(id -g) it make it also pretty safe to do as well

I have considered this and don't like this approach, because it makes the tests work in significantly different environment than the target one. Users will not necessarily want to mount container user's homedir to a host directory. Before this PR, there were docker tests, and they worked by copying test code to the container and running everything in the container. Tests passed, but just running docker run b2 ls bucket failed, as the container user didn't have read rights to his own homedir, which flew under the radar of all tests. The tested image and the released image were slightly different, enough to invalidate all integration tests.

README.md

Co-authored-by: Maciej Urbański <[email protected]>

mjurbanski-reef · 2023-10-28T11:16:15Z

test/integration/test_b2_command_line.py

+    if is_running_on_docker:
+        pytest.skip('Not supported on Docker')


oh, right. thanks

mjurbanski-reef · 2023-10-28T11:19:26Z

test/integration/test_b2_command_line.py

+    sse_c_key_id = 'user-generated-key-id \nąóźćż\nœøΩ≈ç\nßäöü'
+    if is_running_on_docker:
+        # TODO: fix this once we figure out how to pass env vars with \n in them to docker, docker-compose should work
+        sse_c_key_id = sse_c_key_id.replace('\n', '')


I'm just scared about someone loosing a ton of time on debugging this when they actually need it.

I guess solution in the middle would be to emit logger warning in https://github.com/reef-technologies/B2_Command_Line_Tool/pull/213/files#diff-0457e1c77e315509ddcb03e30c7d249298c33f012541c7e3654b6728e7cf56b5R424 IF any of the vars contains newlines. That way when adding a new test it blows up the warning will be visible in pytest output.

mpnowacki-reef force-pushed the docker2 branch from d92c5b3 to ff29562 Compare October 27, 2023 10:07

build and push docker image on release

9bc8ff9

mpnowacki-reef force-pushed the docker2 branch from ff29562 to 9bc8ff9 Compare October 27, 2023 10:56

mpnowacki-reef changed the title ~~pushing docker test~~ build and push docker on release Oct 27, 2023

mjurbanski-reef reviewed Oct 27, 2023

View reviewed changes

mpnowacki-reef and others added 4 commits October 27, 2023 21:56

Update README.md

b5db4ff

Co-authored-by: Maciej Urbański <[email protected]>

post review fixes

d049590

lint

895f007

post review fix

df6878a

mjurbanski-reef approved these changes Oct 28, 2023

View reviewed changes

post review fix

c96f71b

mpnowacki-reef force-pushed the docker2 branch from ac1761b to c96f71b Compare October 28, 2023 20:01

mpnowacki-reef closed this Oct 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build and push docker on release #213

build and push docker on release #213

mpnowacki-reef commented Oct 21, 2023

mjurbanski-reef left a comment

mjurbanski-reef Oct 27, 2023

mjurbanski-reef Oct 27, 2023

mjurbanski-reef Oct 27, 2023

mpnowacki-reef Oct 27, 2023

mjurbanski-reef Oct 27, 2023

mpnowacki-reef Oct 27, 2023

mjurbanski-reef Oct 27, 2023

mpnowacki-reef Oct 27, 2023

mjurbanski-reef Oct 28, 2023

mjurbanski-reef Oct 27, 2023

mpnowacki-reef Oct 27, 2023

mjurbanski-reef Oct 27, 2023

mjurbanski-reef Oct 27, 2023

mpnowacki-reef Oct 27, 2023

mjurbanski-reef Oct 28, 2023

mjurbanski-reef Oct 27, 2023

mpnowacki-reef Oct 28, 2023

mjurbanski-reef Oct 28, 2023

mjurbanski-reef Oct 28, 2023

		if is_running_on_docker:
		pytest.skip('Not supported on Docker')

build and push docker on release #213

build and push docker on release #213

Conversation

mpnowacki-reef commented Oct 21, 2023

mjurbanski-reef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment