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

PP-11224: Add Dockerfile, config, and unit test workflow #2

Merged
merged 7 commits into from
Jul 21, 2023

Conversation

jfharden
Copy link
Contributor

@jfharden jfharden commented Jul 19, 2023

Add initial Dockerfile, config, and a workflows to test and tag releases.

You can test this locally by going into the tests/ directory and running ./test.sh (You will need jq installed)

@jfharden
Copy link
Contributor Author

I tried to add otel-config-validator to test the config however....

  1. it doesn't understand the sigv4auth extension so sees it as an error (opened Add support for sigv4auth extension lightstep/otel-config-validator#8 to suggest it as a feature)
  2. It doesn't exit with a bad exitcode when validation fails so it's a bit fragile to detect the errors (opened Add option to exit non-zero if there's an error lightstep/otel-config-validator#9 with a feature request to add a fail on error flag to the cli)

@jfharden jfharden force-pushed the pp-11224-add-initial-dockerfile branch from a233fae to a5a4512 Compare July 20, 2023 11:24
@jfharden jfharden marked this pull request as ready for review July 20, 2023 11:27
- uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab
- name: Docker build
run: |
docker build -t pay-adot:github-tests .
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we tag the build with the SHA (like we do on the telegraf tests)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's needed, we're not actually putting this image anywhere so the tag is completely ephemeral anyway

"express": "^4.18.2",
"prom-client": "^14.2.0"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add config for these npm packages into dependabot.yml? (The usual open-pull-requests-limit: 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, totally forgot about the npm deps (I wasn't going to have any until the otel config validator didn't work :()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 094eabd

@@ -0,0 +1,68 @@
import { randomUUID } from 'crypto'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it's not anymore 🤔 I did have it previously and decided it was pointless. I'll remove this

katstevens
katstevens previously approved these changes Jul 21, 2023
Copy link
Contributor

@katstevens katstevens left a comment

Choose a reason for hiding this comment

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

Been playing around with the tests locally, looks good! I ran into a minor bit of confusion when trying to make the tests fail on purpose, but realised it was because docker-compose hadn't rebuilt the test-app image for me (and I had to delete it manually). In practice I think this will be fine - GHA will not have any cached images lying around.

@jfharden
Copy link
Contributor Author

Been playing around with the tests locally, looks good! I ran into a minor bit of confusion when trying to make the tests fail on purpose, but realised it was because docker-compose hadn't rebuilt the test-app image for me (and I had to delete it manually). In practice I think this will be fine - GHA will not have any cached images lying around.

Yeah I think I will add docker-compose build to the start of the test script. It will rebuild the images using the normal docker caching rules instead of not rebuilding at all

@jfharden
Copy link
Contributor Author

@katstevens I've done the couple of updates (removed the unused import, added the docker-compose build). Would you mind re-reviewing?

@jfharden jfharden merged commit 8214494 into main Jul 21, 2023
@jfharden jfharden deleted the pp-11224-add-initial-dockerfile branch July 21, 2023 09:52
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