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

Template updates #8

Merged
merged 13 commits into from
Dec 4, 2024
Merged

Template updates #8

merged 13 commits into from
Dec 4, 2024

Conversation

miker985
Copy link
Contributor

@miker985 miker985 commented Dec 3, 2024

Hopefully some innovation ...

https://github.com/UWIT-IAM/innovate-tagging is the example I went with, which works though it is only deployed to dev. This means you can run the "deploy" workflow to dev or eval and it should do the right thing. Prod would fail.

This is the GAR perspective of what the images look like
image

Outcomes

  1. Remove use of fingerprinter
  2. Retags existing X.Y.Z images and omits DEPLOYMENT_ID instead of building new derivative images
  3. More idiot-proofing for creating a new application and having any hope of it working

@miker985 miker985 requested a review from counik December 3, 2024 23:24
@@ -3,7 +3,7 @@ name: Finalize Template
on:
workflow_dispatch:
inputs:
app-name:
app_name:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bugfix - caused the provided app name to always be ignored due to failed lookup

@@ -11,43 +11,40 @@ def get_parser():
"Fills out argument templates, then destroys, saves the new files,"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This source file got "blackened" and thus the changes are pretty noisy. The key here is in (new) lines 41-47 which define an app_name_hyphen and app_name_underscore so things can be more explicitly provided in a way that works regardless of what you named the repository

@@ -51,19 +51,40 @@ jobs:
- name: Install package
run: poetry install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flagging that I got a PermissionError here on my test innovate_tagging project. I side-stepped it by commenting out poetry config virtualenvs.create false on the "Install and configure poetry" task

eval)
# substitute '_' for '-' in APP_NAME
url="https://${1//_/-}.iamdev.s.uw.edu/status"
version=$(curl --silent $url | python -c "import json, sys; print(json.load(sys.stdin)['version'])")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you hate this and want jq back I can do that

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm totally good with this

us-docker.pkg.dev/uwit-mci-iam/containers/${template:app_name}:${{ steps.get-version.outputs.version }}
us-docker.pkg.dev/uwit-mci-iam/containers/${template:app_name}:${{ env.DEPLOYMENT_ID }}
us-docker.pkg.dev/uwit-mci-iam/containers/${template:app_name_hyphen}:${{ steps.get-version.outputs.version }}
us-docker.pkg.dev/uwit-mci-iam/containers/${template:app_name_hyphen}:${{ env.DEPLOYMENT_ID }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on your comment in the body of the PR, didn't you intend to remove DEPLOYMENT_ID from this?

It will also need to be removed from the Dockerfile template.

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 think I successfully removed all the DEPLOYMENT_ID stuff

@miker985 miker985 requested a review from counik December 4, 2024 17:43
return [
str(path.joinpath()) for path in list(Path('.').rglob('*.template.*'))
]
return [str(path.joinpath()) for path in list(Path(".").rglob("*.template*"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the trailing "." to "" since for globbing purposes the former REQUIRED a "." which basically meant it would only work on files with extensions ... which Dockerfile lacks

@miker985 miker985 merged commit fe165c5 into main Dec 4, 2024
2 checks passed
@miker985 miker985 deleted the template-updates branch December 4, 2024 18:06
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