-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add/forgejo #609
Add/forgejo #609
Conversation
ad6c0fa
to
1806357
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen multiple places where Codey is named "VSHNCodey" or "Codey by VSHN". This is wrong, the service is called just "Codey", no VSHN in it. Also see https://kb.vshn.ch/app-catalog/service/forgejo-codey/architecture.html#_codey for how the claim should look like, especially apiVersion
and kind
.
f7f3776
to
59e1516
Compare
@tobru @TheBigLee @zugao I need review here before merging |
class/defaults.yml
Outdated
@@ -58,7 +61,7 @@ parameters: | |||
appcat: | |||
registry: ghcr.io | |||
repository: vshn/appcat | |||
tag: v4.122.0 | |||
tag: add/forgejo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to change that to the correct version
@@ -20,6 +20,7 @@ local getFunction(name, package, runtimeConfigName) = { | |||
}, | |||
spec: { | |||
package: package, | |||
packagePullPolicy: 'IfNotPresent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be configurable. Especially for local testing, where we want to set it to "Always"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I have the same feeling, but I'm not sure if this is the correct place to make such change, maybe a followup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep this as is for now. The appcat function works the same if I am not wrong.
tests/e2e/forgejo/00-install.yaml
Outdated
service: | ||
adminEmail: "[email protected]" | ||
fqdn: | ||
- "somesuperingressname.apps.lab-cloudscale-rma-0.appuio.cloud" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would choose a more useful fqdn eg: forgejo-e2e.apps.lab-cloudscale-rma-0.appuio.cloud
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
tests/e2e/forgejo/scripts/ingress.sh
Outdated
fqdn=$(kubectl -n "$ns" get ingress -oyaml | yq -r '.items.[0].spec.tls.[0].hosts[0]') | ||
|
||
echo "$fqdn = somesuperingressname.apps.lab-cloudscale-rma-0.appuio.cloud" | ||
[[ "$fqdn" == "somesuperingressname.apps.lab-cloudscale-rma-0.appuio.cloud" ]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, see above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to discuss some things
class/defaults.yml
Outdated
forgejo: | ||
registry: ghcr.io | ||
image: vshn/forgejo | ||
tag: latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will probably restart instances once new version is available, we should pin to the latest symver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's copy-paste leftover, I just cleaned it up
defaultPlan: small | ||
sla: 99.25 | ||
plans: | ||
mini: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have these plans been agreed already? @tobru
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -951,7 +1023,47 @@ parameters: | |||
cpu: "2" | |||
memory: "8Gi" | |||
disk: 16Gi | |||
|
|||
codey: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the tfact that we are considering Codey as a service like any other service we already offer. Codey is a specific implementation of Forgejo so we need to discuss whether we want to differentiate it in the component from the normal services. For instance we should consider to put it under forgeo
or maybe under abstracService
instead of service
. Also if we agreed that Codey has nothing to do with VSHN that it might not make sense to leave it under vshn at all (appcat.services.vshn). THis has to be discussed asap before it's too late.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good point and I fully agree, but we have our workflow in component and it expects services to be structured in a specific way, if we want to do any change, it's another story, another ticket and it requires a team-wide decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets keep it this way for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adding base Forgejo and Codey services, claim examples for easy testing: