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

issue #46: Add every useful workflow for this repository #49

Conversation

ThomasCardin
Copy link
Member

@ThomasCardin ThomasCardin commented Jan 16, 2024

The current workflow (build and push docker images to this organisation registry) is working but the workflow it self is missing steps.

Workflow included:

  • Lint test
  • Markdown validation
  • ai-cfia repo standard
  • build and push to github container registry (GCR)

@ThomasCardin ThomasCardin changed the title issue #46: Added every usefull workflow for this repository issue #46: Add every usefull workflow for this repository Jan 16, 2024
Copy link

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

also fix markdown check fails

.github/workflows/lint-test.yml Outdated Show resolved Hide resolved
@ThomasCardin
Copy link
Member Author

also fix markdown check fails

it's working now, but failing for another reason

@rngadam
Copy link

rngadam commented Jan 17, 2024

also fix markdown check fails

it's working now, but failing for another reason

Would it be a good idea to turn this into Draft until you've fixed the failing checks?

@ThomasCardin ThomasCardin marked this pull request as draft January 22, 2024 20:21
@ThomasCardin ThomasCardin self-assigned this Jan 25, 2024
@ThomasCardin ThomasCardin marked this pull request as ready for review January 25, 2024 16:31
@rngadam rngadam changed the title issue #46: Add every usefull workflow for this repository issue #46: Add every useful workflow for this repository Jan 25, 2024
@rngadam
Copy link

rngadam commented Jan 25, 2024

@ThomasCardin spelling of useful is one l


COPY ./requirements.txt .

RUN pip3 install --no-cache-dir -r requirements.txt

COPY . ./

CMD hypercorn -b :$PORT app:app
CMD hypercorn -b :8080 app:app
Copy link

Choose a reason for hiding this comment

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

PORT should stay as GCR (Google Cloud Run) expects PORT to be specifiable (although it does default to 8080).

https://cloud.google.com/run/docs/container-contract

The ingress container within an instance must listen for requests on 0.0.0.0 on the port to which requests are sent. By default, requests are sent to 8080, but you can configure Cloud Run to send requests to the port of your choice. Cloud Run injects the PORT environment variable into the ingress container.

Copy link
Member Author

@ThomasCardin ThomasCardin Jan 31, 2024

Choose a reason for hiding this comment

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

Okay, but we don't use GCR ❓So we don't need to keep the $PORT.

Copy link

Choose a reason for hiding this comment

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

Why would you not have this though? Seems to be a good practice to have a default value for PORT but allow it to be overriden too.

* **NACHET_AZURE_STORAGE_CONNECTION_STRING**: Connection string to access
external storage (Azure Blob Storage).
* **NACHET_MODEL_ENDPOINT_REST_URL**: Endpoint to communicate with deployed
model for inferencing.
* **NACHET_MODEL_ENDPOINT_ACCESS_KEY**: Key used when consuming online endpoint.
* **NACHET_DATA**: Url to access nachet-data repository
Copy link

Choose a reason for hiding this comment

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

URL

Copy link
Member Author

Choose a reason for hiding this comment

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

Not related to this issue.

Copy link

Choose a reason for hiding this comment

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

we can still fix typos when we open a file. IMO, this is better than having to open PR to fix only typos. We leave things we modify in a better state than we got it.

.mlc_config.json Outdated Show resolved Hide resolved
@ThomasCardin
Copy link
Member Author

ThomasCardin commented Jan 31, 2024

@rngadam ping. I'm waiting for this pull request before I can proceed with this one: ai-cfia/howard#4. (change de default tag of the docker image inside each kubernetes deployment to main see (ai-cfia/howard#4 (comment)))

@rngadam
Copy link

rngadam commented Feb 4, 2024

@rngadam ping. I'm waiting for this pull request before I can proceed with this one: ai-cfia/infra#4. (change de default tag of the docker image inside each kubernetes deployment to main see (ai-cfia/infra#4 (comment)))

the test is failing (lint-test step failing)

@ThomasCardin
Copy link
Member Author

Out of scope of this issue

@rngadam
Copy link

rngadam commented Feb 9, 2024

Out of scope of this issue

it is not out of scope; tests need to be green in a pull request before it gets merged.

@MaxenceGui
Copy link

MaxenceGui commented Mar 12, 2024

The error was coming from using a Mock object instead of a MagicMock object (to be able to use the __iter__ ) function. It was solved on

Does this PR still need to be pushed to main, or are the workflows being pushed already?

@ThomasCardin @rngadam

@rngadam
Copy link

rngadam commented Mar 19, 2024

The error was coming from using a Mock object instead of a MagicMock object (to be able to use the __iter__ ) function. It was solved on

Does this PR still need to be pushed to main, or are the workflows being pushed already?

@ThomasCardin @rngadam

@ThomasCardin since this is fixed in main, this pull request should be rebased to get the tests to pass to allow for merging.

@MaxenceGui MaxenceGui added the devSecOps Issue open by the devSecOps teams label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devSecOps Issue open by the devSecOps teams
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants