Skip to content
This repository was archived by the owner on Mar 31, 2022. It is now read-only.

create a test that fails to demonstrate issue #75#82

Open
ryangardner wants to merge 3 commits intospotify:masterfrom
ryangardner:master
Open

create a test that fails to demonstrate issue #75#82
ryangardner wants to merge 3 commits intospotify:masterfrom
ryangardner:master

Conversation

@ryangardner
Copy link
Copy Markdown

Note: in its current form, this test fails... I haven't worked on a fix for it, but figured an IT that failed might make it easier to fix the issue reported in #75

@ryangardner
Copy link
Copy Markdown
Author

Hmm... nevermind. this test doesn't demonstrate this issue yet. I'll try to improve it so it does.

@ryangardner
Copy link
Copy Markdown
Author

This latest PR also adds a simple fix for the issue identified... the test metadata method tries to copy everything from the docker info directory over - if that directory is used for something else and has folders in it, the copy will fail.

It seems like only copying the set of expected files over would be a better behavior, because it's hard to detect if someone is putting files in that directory that shouldn't be there - but if there is a directory there in this PR it will be detected and a suggestion on how to configure the plugin will be logged at the warning level

Copy link
Copy Markdown
Member

@mattnworb mattnworb 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 PR and for aiming to reproduce this in a test case.

I'd prefer if this issue was addressed by validating at the start of the plugin execution that contextDirectory and dockerInfoDirectory were not colliding or overlapping, as I think that would probably cause additional headaches or weird behavior beyond the writeTestMetadata exception.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants