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

build(docker): Release 0.5.0 #473

Closed
wants to merge 21 commits into from
Closed

Conversation

lrcouto
Copy link
Contributor

@lrcouto lrcouto commented Dec 12, 2023

Description

Release kedro-docker 0.5.0

Development notes

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes

@astrojuanlu
Copy link
Member

@astrojuanlu
Copy link
Member

Somehow the logs are wrapped in a more narrow max width now and that's why some assertions fail. For example one test expects INFO - Pipeline execution completed successfully but instead the logs show

                          INFO     Pipeline execution completed          runner.py:112
                                   successfully.

(notice how the line is broken)

https://github.com/kedro-org/kedro-plugins/actions/runs/7186100161/job/19570771296?pr=473#step:7:168

@AhdraMeraliQB
Copy link
Contributor

Somehow the logs are wrapped in a more narrow max width now and that's why some assertions fail. For example one test expects INFO - Pipeline execution completed successfully but instead the logs show

                          INFO     Pipeline execution completed          runner.py:112
                                   successfully.

(notice how the line is broken)

https://github.com/kedro-org/kedro-plugins/actions/runs/7186100161/job/19570771296?pr=473#step:7:168

Might be helpful: https://stackoverflow.com/questions/14446861/matching-discontinuous-interrupted-strings

@noklam
Copy link
Contributor

noklam commented Dec 19, 2023

I starting to suspect if this PR accidentally bring in a bug #92, I am not certain because before that our e2e-test never try to write on mounted storage. pandas-iris only read from it but now we changer to spaceflights-pandas which write to the mounted volume.

The problem only appear when using with the --uid and --gid options.

@merelcht
Copy link
Member

I starting to suspect if this PR accidentally bring in a bug #92, I am not certain because before that our e2e-test never try to write on mounted storage. pandas-iris only read from it but now we changer to spaceflights-pandas which write to the mounted volume.

The problem only appear when using with the --uid and --gid options.

I propose we do the following:

  1. Create a simpler test started for kedro-docker e2e tests that doesn't need to write to mounted volume
  2. In a separate task fix the issue with mounted volume in combination with the --uid and --gid options

It would be a shame to have the compatibility with Kedro 0.19.0 blocked by this issue which might have been broken for a much longer time already.

@noklam
Copy link
Contributor

noklam commented Dec 19, 2023

@merelcht After reading this carefully, I have changed my mind and I don't think it is a broken feature, it is just a bad test.

Create a simpler test started for kedro-docker e2e tests that doesn't need to write to mounted volume

I agree with this. The permission problem is not a bug because UID/GID need to match with the host. For someone who set this manually should be aware of the consequence.

I propose we update the test. Instead of testing with a pipeline run, we can use kedro docker ipython, and assert PID and UID to see if it match with the CLI arguments kedro docker ipython. The original test isn't really testing UID/GID, it could still work even if the UID and GID wasn't set properly.

@lrcouto lrcouto marked this pull request as ready for review December 20, 2023 18:42
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

The changes look good! Great work fixing it ⭐

I would suggest doing the release in a separate PR though, to make it easier to trace back the different changes. The release PR should ideally just be the version bump + release notes.

@lrcouto lrcouto closed this Dec 21, 2023
@lrcouto lrcouto deleted the release/kedro-docker/0.5.0 branch December 21, 2023 14:00
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.

5 participants