Skip to content

Test fixup#49

Merged
charliemirabile merged 14 commits into
masterfrom
test-fixup
Mar 27, 2024
Merged

Test fixup#49
charliemirabile merged 14 commits into
masterfrom
test-fixup

Conversation

@theyoyojo

Copy link
Copy Markdown
Contributor

Some small tweaks and fixes to the test.sh script

@charliemirabile charliemirabile left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you remove the stuff that sets up a venv / enters it? You can rely on the user doing that / having their dev environment setup imo. If you want to avoid any nasty surprises/confusing error messages, you can add a check for whether there is a flake8 command (and also probably chcon, curl, and podman)

@charliemirabile charliemirabile left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only thing I am actually requesting to change 100% is the order of the chcon check / use since currently it is completely useless, but if you have to do a rebase, I don't think any of the other things I am suggesting are hard to do.

Final nitpick: I think #!/bin/bash is a bit sus. I think only #!/bin/sh is guaranteed to work, bash is probably in /usr/bin (which on fedora /bin is a symlink to which is why this works), but I think the better solution is #!/usr/bin/env bash; also maybe run this and the other scripts (e.g. test style.sh) through shellcheck and maybe do that automatically as part of the testing process. Each popped a few suggestions when I tried them, nothing major, but good fixes imo.

Comment thread test.sh Outdated
Comment thread test.sh
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Since warpdrive defaults are now sane, simply use them directly. When
testing on prod/staging, sudo is needed and this matches current
invocation of podman compose.

Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>

@charliemirabile charliemirabile left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@charliemirabile charliemirabile merged commit ba3b47a into master Mar 27, 2024
@charliemirabile charliemirabile deleted the test-fixup branch March 27, 2024 00:16
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.

2 participants