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

Bug: target directory should be tidy #227

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

cndoit18
Copy link
Contributor

@cndoit18 cndoit18 commented Oct 28, 2022

Fixes: #59
Signed-off-by: cndoit18 [email protected]

@cndoit18 cndoit18 force-pushed the fix-issue-59 branch 10 times, most recently from 831bc16 to f32258a Compare November 1, 2022 02:56
@cndoit18
Copy link
Contributor Author

cndoit18 commented Nov 1, 2022

PTAL @jekkel

@cndoit18 cndoit18 changed the title fix(tidy): target directory should be tidy Bug target directory should be tidy Nov 3, 2022
@cndoit18 cndoit18 changed the title Bug target directory should be tidy Bug: target directory should be tidy Nov 4, 2022
Copy link
Member

@jekkel jekkel left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply.
I think the approach is sane and it seems to properly track the keys 👍
Unfortunately the code has got a bit to complex now with many calls with huge parameter lists all over the place. Any idea how to improve on that?

Also please add an appropriate test so we catch any regression in the future.

src/resources.py Show resolved Hide resolved
@cndoit18
Copy link
Contributor Author

cndoit18 commented Nov 7, 2022

Sorry for the late reply. I think the approach is sane and it seems to properly track the keys 👍 Unfortunately the code has got a bit to complex now with many calls with huge parameter lists all over the place. Any idea how to improve on that?

Also please add an appropriate test so we catch any regression in the future.

I've tried to simplify it, but I can't think of a suitable way.

.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
src/resources.py Show resolved Hide resolved
jekkel
jekkel previously approved these changes Nov 8, 2022
Copy link
Member

@jekkel jekkel left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. The tests still fail due to bash not available in the image. Maybe you should go with downloading the files before the assertion statement which only looks on local files then as done with all the other assertions and files.

@cndoit18
Copy link
Contributor Author

cndoit18 commented Nov 8, 2022

Thanks for the contribution. The tests still fail due to bash not available in the image. Maybe you should go with downloading the files before the assertion statement which only looks on local files then as done with all the other assertions and files.

Thx, I'll come by later and fix the test part, unfortunately I don't have a way to automatically trigger the ci to determine if my test can run

@cndoit18
Copy link
Contributor Author

cndoit18 commented Nov 8, 2022

Thanks for the contribution. The tests still fail due to bash not available in the image. Maybe you should go with downloading the files before the assertion statement which only looks on local files then as done with all the other assertions and files.

done

@cndoit18 cndoit18 requested a review from jekkel November 8, 2022 10:26
jekkel
jekkel previously approved these changes Nov 8, 2022
@cndoit18
Copy link
Contributor Author

cndoit18 commented Nov 8, 2022

@jekkel A few use cases have been successful and we may need to wait longer to update the data.

@jekkel
Copy link
Member

jekkel commented Nov 9, 2022

[...] unfortunately I don't have a way to automatically trigger the ci to determine if my test can run

Sorry for the inconvenience but for the time being I try to kick-off CI whenever I notice any change on the PR.

@cndoit18
Copy link
Contributor Author

cndoit18 commented Nov 9, 2022

[...] unfortunately I don't have a way to automatically trigger the ci to determine if my test can run

Sorry for the inconvenience but for the time being I try to kick-off CI whenever I notice any change on the PR.

Thank you very much, it looks like it passed

@jekkel jekkel added the enhancement New feature or request label Nov 9, 2022
@jekkel jekkel merged commit cb24baf into kiwigrid:master Nov 9, 2022
@cndoit18 cndoit18 deleted the fix-issue-59 branch November 9, 2022 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renaming key in config map causes duplicate files to exist
2 participants