Skip to content

Add tag config for server/ui/admintools images #789

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.

Sign up for GitHub

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

Open
chulkilee wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add tag config for server/ui/admintools images #789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions api/v1beta1/temporalcluster_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ const (
defaultTemporalVersion = "1.23.0"
defaultTemporalImage = "temporalio/server"

defaultTemporalUIVersion = "2.25.0"
defaultTemporalUIImage = "temporalio/ui"
defaultTemporalUIImage = "temporalio/ui"
defaultTemporalUITag = "2.25.0"

defaultTemporalAdmintoolsImage = "temporalio/admin-tools"
defaultTemporalAdmintoolsTag = "1.23.1.1-tctl-1.18.1-cli-0.12.0"
)

// Default set default fields values.
Expand Down Expand Up @@ -64,6 +65,9 @@ func (c *TemporalCluster) Default() {
if c.Spec.Image == "" {
c.Spec.Image = defaultTemporalImage
}
if c.Spec.Tag == "" {
c.Spec.Tag = c.Spec.Version.String()
}

if c.Spec.Log == nil {
c.Spec.Log = new(LogSpec)
Expand Down Expand Up @@ -193,8 +197,12 @@ func (c *TemporalCluster) Default() {
c.Spec.UI = new(TemporalUISpec)
}

if c.Spec.UI.Version == "" {
c.Spec.UI.Version = defaultTemporalUIVersion
if c.Spec.UI.Tag == "" {
if c.Spec.UI.Version != "" {
c.Spec.UI.Tag = c.Spec.UI.Version
} else {
c.Spec.UI.Tag = defaultTemporalUITag
}
}

if c.Spec.UI.Image == "" {
Expand All @@ -213,6 +221,10 @@ func (c *TemporalCluster) Default() {
c.Spec.AdminTools.Image = defaultTemporalAdmintoolsImage
}

if c.Spec.AdminTools.Tag == "" {
c.Spec.AdminTools.Tag = defaultTemporalAdmintoolsTag
}

if c.Spec.MTLS != nil {
if c.Spec.MTLS.RefreshInterval == nil {
c.Spec.MTLS.RefreshInterval = &metav1.Duration{Duration: time.Hour}
Expand Down
12 changes: 11 additions & 1 deletion api/v1beta1/temporalcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,12 +550,16 @@ type TemporalUISpec struct {
// Enabled defines if the operator should deploy the web ui alongside the cluster.
// +optional
Enabled bool `json:"enabled"`
// Version defines the temporal ui version the instance should run.
// Deprecated. use `tag` instead.
// +optional
// +deprecated
Version string `json:"version"`
// Image defines the temporal ui docker image the instance should run.
// +optional
Image string `json:"image"`
// Tag defines the temporal ui docker image tag the instance should run.
// +optional
Tag string `json:"tag"`
// Number of desired replicas for the ui. Default to 1.
// +kubebuilder:validation:Minimum=1
// +optional
Expand Down Expand Up @@ -585,6 +589,9 @@ type TemporalAdminToolsSpec struct {
// Image defines the temporal admin tools docker image the instance should run.
// +optional
Image string `json:"image"`
// Tag defines the temporal admin tools docker image tag the instance should run.
// +optional
Tag string `json:"tag"`
// Compute Resources required by the ui.
// More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
// +optional
Expand Down Expand Up @@ -1002,6 +1009,9 @@ type TemporalClusterSpec struct {
// Image defines the temporal server docker image the cluster should use for each services.
// +optional
Image string `json:"image"`
// Tag defines the temporal server docker image tag the cluster should use for each services.
// +optional
Tag string `json:"tag"`
Copy link
Copy Markdown
Owner

@alexandrevilain alexandrevilain Aug 17, 2024

Choose a reason for hiding this comment

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

IMO adding .spec.tag in the API can be confusing for end users.
The .spec.version is used as a source of truth on the whole codebase. Please remove this.

Copy link
Copy Markdown
Author

@chulkilee chulkilee Aug 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Users of this operator may not be able to change the convention of container images. Therefore making assumption that tag will be same with version is not desired.

For example I have to use x.y version with custom repo & tag (x.y-yyyymmdd) then there is no way to do that currently. Adding tag solves the problem.

Also I made it optional, falling back to version value - which keeps the current behavior.

Another approach I considered is to introduce image struct with repo and tag fields - instead of having them flattened in the spec. We may move image related fields to it (e.g. pull policy and cred etc). I think this approach is much easier to understand but requires more changes... if you think this is better then I'll try that.

Having image spec for server component explicitly would be more flexible for future changes (e.g what if temporal introducing other components?).

Copy link
Copy Markdown
Owner

@alexandrevilain alexandrevilain Aug 20, 2024

Choose a reason for hiding this comment

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

Ok some please be more explicit on the controller behavior on the .spec.tag godoc.
Please explain that:

  • It's an advanced fields if users wants to use custom builds
  • If empty the operator will use the desired .spec.version

Copy link
Copy Markdown
Author

@chulkilee chulkilee Aug 20, 2024

Choose a reason for hiding this comment

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

Where would be the most appropriate place to document the controller behavior for this field?

While adding it to the Go struct field comment is an option - since it will appear in the CRD description - this s not the place where the actual logic resides. However, including it there might still be beneficial for users who rely on the CRD documentation. Thoughts? @alexandrevilain

Copy link
Copy Markdown
Owner

@alexandrevilain alexandrevilain Aug 21, 2024

Choose a reason for hiding this comment

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

Yep the Go struct field comment is the best place for me IMO, it will be available to the user through:

// Version defines the temporal version the cluster to be deployed.
// This version impacts the underlying persistence schemas versions.
// +optional
Expand Down
10 changes: 6 additions & 4 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion config/crd/bases/temporal.io_temporalclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ spec:
More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
type: object
type: object
tag:
description: Tag defines the temporal admin tools docker image tag the instance should run.
type: string
type: object
archival:
description: Archival allows Workflow Execution Event Histories and Visibility data backups for the temporal cluster.
Expand Down Expand Up @@ -3553,6 +3556,9 @@ spec:
type: object
type: object
type: object
tag:
description: Tag defines the temporal server docker image tag the cluster should use for each services.
type: string
ui:
description: UI allows configuration of the optional temporal web ui deployed alongside the cluster.
properties:
Expand Down Expand Up @@ -3747,8 +3753,11 @@ spec:
(scope and select) objects.
type: object
type: object
tag:
description: Tag defines the temporal ui docker image tag the instance should run.
type: string
version:
description: Version defines the temporal ui version the instance should run.
description: Deprecated. use `tag` instead.
type: string
type: object
version:
Expand Down
50 changes: 49 additions & 1 deletion docs/api/v1beta1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ string
</tr>
<tr>
<td>
<code>tag</code><br>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>Tag defines the temporal server docker image tag the cluster should use for each services.</p>
</td>
</tr>
<tr>
<td>
<code>version</code><br>
<em>
github.com/alexandrevilain/temporal-operator/pkg/version.Version
Expand Down Expand Up @@ -4574,6 +4586,18 @@ string
</tr>
<tr>
<td>
<code>tag</code><br>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>Tag defines the temporal admin tools docker image tag the instance should run.</p>
</td>
</tr>
<tr>
<td>
<code>resources</code><br>
<em>
<a href="https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.23/#resourcerequirements-v1-core">
Expand Down Expand Up @@ -4787,6 +4811,18 @@ string
</tr>
<tr>
<td>
<code>tag</code><br>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>Tag defines the temporal server docker image tag the cluster should use for each services.</p>
</td>
</tr>
<tr>
<td>
<code>version</code><br>
<em>
github.com/alexandrevilain/temporal-operator/pkg/version.Version
Expand Down Expand Up @@ -6027,7 +6063,7 @@ string
</td>
<td>
<em>(Optional)</em>
<p>Version defines the temporal ui version the instance should run.</p>
<p>Deprecated. use <code>tag</code> instead.</p>
</td>
</tr>
<tr>
Expand All @@ -6044,6 +6080,18 @@ string
</tr>
<tr>
<td>
<code>tag</code><br>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>Tag defines the temporal ui docker image tag the instance should run.</p>
</td>
</tr>
<tr>
<td>
<code>replicas</code><br>
<em>
int32
Expand Down
2 changes: 1 addition & 1 deletion internal/resource/admintools/deployment_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func (b *DeploymentBuilder) Update(object client.Object) error {
Containers: []corev1.Container{
{
Name: "admintools",
Image: fmt.Sprintf("%s:%s", b.instance.Spec.AdminTools.Image, b.instance.Spec.Version),
Image: fmt.Sprintf("%s:%s", b.instance.Spec.AdminTools.Image, b.instance.Spec.AdminTools.Tag),
ImagePullPolicy: corev1.PullIfNotPresent,
TerminationMessagePath: corev1.TerminationMessagePathDefault,
TerminationMessagePolicy: corev1.TerminationMessageReadFile,
Expand Down
2 changes: 1 addition & 1 deletion internal/resource/base/deployment_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ func (b *DeploymentBuilder) Update(object client.Object) error {
Containers: []corev1.Container{
{
Name: "service", // name "service" is here to simplify overrides
Image: fmt.Sprintf("%s:%s", b.instance.Spec.Image, b.instance.Spec.Version),
Image: fmt.Sprintf("%s:%s", b.instance.Spec.Image, b.instance.Spec.Tag),
ImagePullPolicy: corev1.PullIfNotPresent,
Resources: b.service.Resources,
TerminationMessagePath: corev1.TerminationMessagePathDefault,
Expand Down
2 changes: 1 addition & 1 deletion internal/resource/persistence/schema_setup_job_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func (b *SchemaJobBuilder) Build() client.Object {
Containers: []corev1.Container{
{
Name: "schema-script-runner",
Image: fmt.Sprintf("%s:%s", b.instance.Spec.AdminTools.Image, b.instance.Spec.Version),
Image: fmt.Sprintf("%s:%s", b.instance.Spec.AdminTools.Image, b.instance.Spec.AdminTools.Tag),
ImagePullPolicy: corev1.PullIfNotPresent,
Resources: b.instance.Spec.JobResources,
TerminationMessagePath: corev1.TerminationMessagePathDefault,
Expand Down
2 changes: 1 addition & 1 deletion internal/resource/ui/deployment_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func (b *DeploymentBuilder) Update(object client.Object) error {
Containers: []corev1.Container{
{
Name: "ui",
Image: fmt.Sprintf("%s:%s", b.instance.Spec.UI.Image, b.instance.Spec.UI.Version),
Image: fmt.Sprintf("%s:%s", b.instance.Spec.UI.Image, b.instance.Spec.UI.Tag),
ImagePullPolicy: corev1.PullIfNotPresent,
Resources: b.instance.Spec.UI.Resources,
TerminationMessagePath: corev1.TerminationMessagePathDefault,
Expand Down
1 change: 0 additions & 1 deletion pkg/version/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.