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

[Open Media Matcher] GitHub Actions Setup #1352

Merged
merged 6 commits into from
Aug 30, 2023

Conversation

juanmrad
Copy link
Collaborator

Summary

Adding lint and test dependencies to the project as well as initial test and GitHub Actions settings for the Open Media Matcher.

Test Plan

Test run on CI now.

cc/ #1351

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for doing it!

Let me just check with Doug to make sure there isn't additional considerations or things he sees that I can't

@@ -19,6 +19,15 @@ dependencies = [
"flask",
]

[project.optional-dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

Lol, @dougneal , who I think removed the all alias in a previous version.

To toss my hat in the ring:

  1. I think it's helpful to separate test and production dependencies
  2. Where I had landed on with Doug is that it would be fine to just have installing anything be the default
  3. I am okay with having the test/all split brought back, but defer to @dougneal for additional opinons.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would definitely expect the dev mode dependencies to be different to the production mode dependencies, so this is expected in some form or another.

The pyproject.toml way of doing things has a built-in concept of build-time dependencies. This means these tools are listed under the requires section of the build-system stanza (above). This makes it more explicit that "these deps are for dev/build time" whereas putting them under optional dependencies is a bit more ambiguous in its intent.

The pyproject.toml can also contain configuration stanzas for each of these tools, see this example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but wouldn't adding them all to the build-system mean it will always be installed? we wouldn't want them installed on the prod container tho. unless you mean to create one for prod that overrides the dependencies with a smaller subset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dougneal I'm not sure I am able to follow how to get this working without the use of optional dependencies. I'm having a hard time understanding how to get the dependencies to be part of the build-system such as they are installed on --editable 😓 .

Copy link
Contributor

Choose a reason for hiding this comment

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

pls ignore my nonsense 🙂

@app.route('/')
app.config.from_envvar("OMM_CONFIG")

@app.route("/")
Copy link
Contributor

Choose a reason for hiding this comment

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

@dougneal - this is going to create collisions with your outstanding diffs, but there's no way around it, and we should be innoculated going forward.

Copy link
Contributor

@dougneal dougneal left a comment

Choose a reason for hiding this comment

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

This is great, thanks! Just a few feedback points on the dependencies stuff.

.github/workflows/omm-ci.yaml Outdated Show resolved Hide resolved
@@ -19,6 +19,15 @@ dependencies = [
"flask",
]

[project.optional-dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would definitely expect the dev mode dependencies to be different to the production mode dependencies, so this is expected in some form or another.

The pyproject.toml way of doing things has a built-in concept of build-time dependencies. This means these tools are listed under the requires section of the build-system stanza (above). This makes it more explicit that "these deps are for dev/build time" whereas putting them under optional dependencies is a bit more ambiguous in its intent.

The pyproject.toml can also contain configuration stanzas for each of these tools, see this example.

open-media-match/mypy.ini Outdated Show resolved Hide resolved
@juanmrad juanmrad requested a review from dougneal August 30, 2023 01:46
@dougneal dougneal merged commit a7a6dd0 into facebook:main Aug 30, 2023
5 checks passed
@juanmrad juanmrad deleted the omm/juanmrad/github-actions-settings branch August 30, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants