Skip to content
This repository has been archived by the owner on Nov 25, 2022. It is now read-only.

Add argo specifications #173

Merged
merged 17 commits into from
Nov 15, 2021
Merged

Add argo specifications #173

merged 17 commits into from
Nov 15, 2021

Conversation

rupeshparab
Copy link
Contributor

@rupeshparab rupeshparab commented Oct 25, 2021 •
edited by jgwerner
Loading

  • Adds Argo manifests for Argo Events and Argo Workflows
  • Argo events and workflows are used to manage auto-grading

@rupeshparab rupeshparab requested a review from jgwerner October 25, 2021 15:52
@rupeshparab rupeshparab self-assigned this Oct 25, 2021
@rupeshparab
Copy link
Contributor Author

rupeshparab commented Oct 25, 2021 •
edited
Loading

the failure in linting seems to be due to this: argoproj/argo-events#562

Will need to make some changes to the spec to make it pass

jgwerner
jgwerner suggested changes Oct 25, 2021
View reviewed changes
Copy link
Member

@jgwerner jgwerner left a comment

Choose a reason for hiding this comment

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

@rupeshparab added some questions, thanks!

charts/illumidesk/Chart.yaml Outdated Show resolved Hide resolved
charts/illumidesk/templates/argo_sensor.yaml Outdated Show resolved Hide resolved
charts/illumidesk/templates/argo_sensor.yaml Show resolved Hide resolved
charts/illumidesk/templates/argo_sensor.yaml Show resolved Hide resolved
@Abhi94N
Copy link
Contributor

Abhi94N commented Oct 25, 2021

Make sure update chart yaml with new semver
helm package charts/illumidesk/ -d docs/ --version {semver}
Index docs
helm repo index docs/

@rupeshparab rupeshparab requested a review from jgwerner October 26, 2021 10:03
Abhi94N
Abhi94N approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@Abhi94N Abhi94N left a comment

Choose a reason for hiding this comment

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

LGTM. After the demo I can update the README

Abhi94N
Abhi94N suggested changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@Abhi94N Abhi94N left a comment

Choose a reason for hiding this comment

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

Specify namespace for anything that is not a cluster based resource. Anything that is not a persistent volume, ClusterRole, ClusterRoleBinding, should have a namespace

rupeshparab
rupeshparab commented Nov 11, 2021
View reviewed changes
charts/illumidesk/templates/argo_sensor.yaml
- name: grader-pv-{{ .Release.Namespace }}
mountPath: /home
subPath: illumidesk-courses/{{ .Release.Namespace }}/home
- name: shared-pv-{{ .Release.Namespace }}
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 can be removed

jgwerner
jgwerner approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member

@jgwerner jgwerner left a comment

Choose a reason for hiding this comment

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

LGTM

README.md
## Steps to setup argo on a new cluster

```bash
helm install argo argo/argo-workflows -n argo --create-namespace
Copy link
Member

Choose a reason for hiding this comment

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

@Abhi94N perhaps it's best to abstract this away by deploying Argo with a sub-chart?

NOTE: after today's review (11.11.2021) this will stay in place since it's a cluster-level resource. We have opened issue #175 and that chart could include the Argo setup as a sub-chart.

@jgwerner jgwerner requested a review from Abhi94N November 15, 2021 03:32
@jgwerner jgwerner merged commit 93f28c4 into IllumiDesk:main Nov 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jgwerner jgwerner jgwerner approved these changes

@Abhi94N Abhi94N Awaiting requested review from Abhi94N

Assignees

@rupeshparab rupeshparab

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rupeshparab @Abhi94N @jgwerner