Skip to content

Conversation

@TGPSKI
Copy link
Contributor

@TGPSKI TGPSKI commented Jun 25, 2025

This pull request updates the schema file schemas/app-sre/saas-file-2.yml to include a title, description for the top-level schema, and descriptions for all properties. All descriptions use multiline YAML syntax.

(Optional) A list of resource kind and names to manage, filtering out
other resources of the same kind. Used in specific cases, like
deploying to a protected namespace (i.e. openshift-* namespaces) while using
elevated permissions (cluster-admin role).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a mention (or rephrase) to state that cluster-wide resources (eg clusterrole/clusterrolebinding) need to have their name set here to avoid conflicts with other saas files deploying the same kind of resources cluster-wide

Copy link
Contributor

@patjlm patjlm Jun 26, 2025

Choose a reason for hiding this comment

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

maybe something like

 Mandatory in specific cases, like
 deploying to a protected namespace (i.e. openshift-* namespaces) while using
 elevated permissions (cluster-admin role) or deploying cluster-wide resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the (Optional) if you're not going to add it to all descriptions or if it doesn't add certain information, e.g. (optional, required for X)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inconsistencies around optional params / default property values injected at the automation level persist. Multiple comments on this topic - I only want to proceed with a consistent approach for all files.

parameters:
type: object
description: |
Parameters to use when deploying this resource template.
Copy link
Contributor

Choose a reason for hiding this comment

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

worth mentioning that these can override saas-file level parameters

description: resource template level parameters from vault secrets
description: |
A list of secret template parameters, injected from Vault, that are applied to
the associated resource template targets.
Copy link
Contributor

Choose a reason for hiding this comment

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

can override saas-file level secretParameters

publishJobLogs:
type: boolean
description: |
If true, publishes job logs as artifacts in the deployment job.
Copy link
Contributor

Choose a reason for hiding this comment

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

not exactly. It will gather the logs of the Job or ClowdJobInvocation being deployed and print them out in the pipelinerun output.

https://github.com/app-sre/qontract-reconcile/blob/master/reconcile/openshift_saas_deploy.py#L266-L271

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 guess the question is "pipelinerun output" considered a build artifact? I understood build logs, validation output, and other outputs from a job to be artifacts.

"* publishJobLogs - (optional) if this is a saas file running post-deployment tests, set this to true to publish Job's pods logs as artifacts in the Jenkins job. This will also ensure that deployments are triggered on config changes in any subscribed publisher target. See promotion_data."

cc https://gitlab.cee.redhat.com/service/app-interface/-/blob/master/docs/app-sre/continuous-delivery-in-app-interface.md?ref_type=heads

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is not true anymore and should be checked.

  • we don't run saas-deploy jobs from jenkins, so there is no artifact publish. Deployment PipelineRun only have their pod logs and this setting does not push them anywhere
  • I think This will also ensure that deployments are triggered on config changes in any subscribed publisher target is wrong as well nowadays. Hasn't been completely replaced by redeployOnPublisherConfigChange in saas targets?

targets:
type: array
description: |
A list of deployment targets for this resource template.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention that in most cases targets can be set inline instead of externally to the saas file

templates, deployment targets, notifications, registry authentication,
secret injection, and more. They enable service owners to declaratively manage
their deployment flow, automate promotions, and integrate with CI/CD
systems such as Tekton, Jenkins, and Konflux.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove Konflux, we don't do anything from saas files to Konflux.

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 can re-word this description, but I think it's important for a future self-service user to understand that saas-file-2 is the connection point between disconnected build and delivery systems.

We expect all future container builds to be via Konflux. saas-file-2 instances declare the relationship between an external build system and auto-promotion triggers. These triggers depend on upstream job success (jenkins) or presence of images in a container registry (Konflux -> Quay).

(Optional) A list of resource kind and names to manage, filtering out
other resources of the same kind. Used in specific cases, like
deploying to a protected namespace (i.e. openshift-* namespaces) while using
elevated permissions (cluster-admin role).
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the (Optional) if you're not going to add it to all descriptions or if it doesn't add certain information, e.g. (optional, required for X)

items:
type: string
description: |
A list of allowed Vault paths for secret parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A list of allowed Vault paths for secret parameters.
A list of allowed Vault paths for secret parameters. Required for `secretParameters`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re: optional -> more default issues, need a consistent approach for all schema files instead of ad-hoc.

- helm
description: |
The type of resource template: OpenShift template, directory of
manifests, or Helm chart.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
manifests, or Helm chart.
manifests, or Helm chart. If not specified, `openshift-template` will be assumed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More defaults, need a consistent approach for all schema files instead of ad-hoc.

hash_length:
type: integer
description: |
The length of the commit hash to use for image tags.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The length of the commit hash to use for image tags.
The length of the commit hash to use for image tags. The default is set via the
`hashLength` property in /app-interface/app-interface-settings-1.yml schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More defaults, need a consistent approach for all schema files instead of ad-hoc.

type: boolean
description: prevent updates to a saas file
description: |
If true, prevents updates to this SaaS file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If true, prevents updates to this SaaS file.
If true, prevents updates to this SaaS file allowing for
resource templates to be migrated to different SaaS files.

# - *.live.dynatrace.com
pattern: '^(quay\.io|registry\.redhat\.io|registry\.connect\.redhat\.com||registry\.access\.redhat\.com|[^.]+\.dkr\.ecr\.[^.]+\.amazonaws\.com|[^.]+\.live\.dynatrace\.com).*$'
description: |
A list of allowed image patterns for deployments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A list of allowed image patterns for deployments.
A list of allowed image patterns for deployments. Please avoid very generic
patterns, such as `quay.io/app-sre` as they make image traceability hard.

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 prefer enforcing this restriction via regex patterns, instead of hoping the user follows good intentions / best practices.

Co-authored-by: Rafael Porres Molina <[email protected]>
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.

3 participants