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

feat: Reorganise workflow yamls #262

Closed
wants to merge 25 commits into from

Conversation

stevekeay
Copy link
Contributor

@stevekeay stevekeay commented Sep 9, 2024

This answers PUC-505

I don't know if this is the right layout, I was just trying to make the workflows consistent with one another.

I opened this as a starting point for a discussion. Needs further work before merging, to fix up paths, etc.

I left the names alone, although there is probably room for improvement there.

I also moved the /argo-workflows/containers into /containers and the existing contents of /containers should move into subfolders like /containers/ironic/. I think there was originally good reason for the separation but they have become mixed up over time.

@@ -80,10 +80,10 @@ def argument_parser():
return parser


def main():
"""Updates connected_to_network and triggers Undersync.
main def():
Copy link
Contributor

Choose a reason for hiding this comment

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

should be def main():?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, thanks fixed in rebase

@skrobul
Copy link
Collaborator

skrobul commented Sep 10, 2024

I also left the /argo-workflows/containers separate from /containers, although the separation doesn't make much sense to me. I think /argo-workflows/containers/* should move to /containers/ and the existing contents of /containers should move into subfolders like /containers/ironic/.

The argo-workflows/**/containers/ structure was created when we had the workflows organised by feature so that we could easily identify which container image is used for which feature. Given that this is no longer the case and the workflows are all lumped into a single folder now, having the separate argo-workflows/containers might not be needed anymore.
In fact, we may be able to eliminate most of these:

  • python311_alpine can be pulled to the /containers
  • python312_alpine is not used anywhere afaik
  • for argo_utils we should look into incorporating the code/argo_python/__init__.py into the understack_workflows's ArgoClient and possibly getting rid of that container image altogether

I don't want to scope creep this, so happy to create separate JIRA for that if you think it's a good idea.

Copy link
Collaborator

@skrobul skrobul left a comment

Choose a reason for hiding this comment

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

I think your current organization looks good overall, but I have a minor suggestion for improvement. The current naming convention is slightly misleading, as the 'workflows' folder implies that it contains Workflow objects, but it actually holds WorkflowTemplates. Would it be a good idea to rename the folder to workflowtemplates to better reflect its contents and avoid confusion?

@stevekeay
Copy link
Contributor Author

I think your current organization looks good overall, but I have a minor suggestion for improvement. The current naming convention is slightly misleading, as the 'workflows' folder implies that it contains Workflow objects, but it actually holds WorkflowTemplates. Would it be a good idea to rename the folder to workflowtemplates to better reflect its contents and avoid confusion?

Good point, I had mis-read the "kind" and I fixed this in 821457f

@stevekeay
Copy link
Contributor Author

I made some further changes, including the suggestions above but not the argo_utils changes.

There are still a few paths to fix up.

@cardoe
Copy link
Contributor

cardoe commented Sep 10, 2024

There's also apps/understack-workflows that would need to be moved back to here.

@stevekeay stevekeay force-pushed the reorganise-workflow-yamls branch 2 times, most recently from cd9e986 to f763e1e Compare September 10, 2024 16:01
Steve Keay added 18 commits September 11, 2024 09:02
We had a hierarchy of workflows that was not being fully utilised.  This
discards that structure in favour of the yaml file being organised by Kind and
then by name.

for i in `grep -lr "^kind: WorkflowTemplate"` ; do
  mv $i workflows/`cat "$i" | yq ".metadata.name"`.yaml
done
for i in `grep -lr "^kind: Sensor"` ; do
  mv $i sensors/`cat "$i" | yq ".metadata.name"`.yaml
done
etc
We don't use this structure any more.
These are separated out into folders by k8s namespace and then by "kind".

Kustomization is added to manage these
@stevekeay stevekeay force-pushed the reorganise-workflow-yamls branch 4 times, most recently from bf25e35 to 31a29e3 Compare September 11, 2024 10:15
When CI runs on a fork it doesn't have permissions to push there
@stevekeay
Copy link
Contributor Author

Closing in favour of #267 which has better CI

@stevekeay stevekeay closed this Sep 11, 2024
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.

4 participants