Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-1290. Add k8s labels to k8s resources in submarine-cloud-v3 #976

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ianshen1104
Copy link

What is this PR for?

Add app.kubernetes.io labels recommended here by Kubernetes
https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/

What type of PR is it?

Improvement

Todos

  • - Add app.kubernetes.io labels in submarine-cloud-v3 artifacts

What is the Jira issue?

https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-1290

How should this be tested?

Follow the submarine-cloud-v3/docs/developer-guide.md

Screenshots (if appropriate)

Questions:

  • Do the license files need updating? No
  • Are there breaking changes for older versions? No
  • Does this need new documentation? No

@cdmikechen
Copy link
Contributor

cdmikechen commented Jul 8, 2022

Hi~ Thanks for your contribution.
Can we replace app.kubernetes.io/version value using a consistent method? I personally don't think hard-coding the version is a good way.
Another question is whether we have any way to test the current updates?

@ianshen1104
Copy link
Author

@cdmikechen thanks for your feedback!
I will update my changes soon and push again.

@codecov
Copy link

codecov bot commented Jul 9, 2022

Codecov Report

Merging #976 (fcfd6a6) into master (e7ed4a5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master    #976   +/-   ##
========================================
  Coverage      9.18%   9.18%           
  Complexity      670     670           
========================================
  Files           234     234           
  Lines         23716   23716           
  Branches       3475    3475           
========================================
  Hits           2178    2178           
  Misses        21407   21407           
  Partials        131     131           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Comment on lines 20 to 27
kind: PersistentVolumeClaim
metadata:
name: submarine-database-pvc
labels:
app.kubernetes.io/name: submarine-database
app.kubernetes.io/version: "0.8.0-SNAPSHOT"
app.kubernetes.io/component: database
spec:
Copy link
Member

Choose a reason for hiding this comment

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

I think we can write the version information in golang file instead of yaml file. For example, for this PersistentVolumeClaim, see the golang function below. We can set pvc.Labels before we create the resource. This version information can be written as golang constant.

func (r *SubmarineReconciler) newSubmarineDatabasePersistentVolumeClaim(ctx context.Context, submarine *submarineapacheorgv1alpha1.Submarine) *corev1.PersistentVolumeClaim {
pvc, err := ParsePersistentVolumeClaimYaml(databaseYamlPath)
if err != nil {
r.Log.Error(err, "ParsePersistentVolumeClaimYaml")
}
pvc.Namespace = submarine.Namespace
err = controllerutil.SetControllerReference(submarine, pvc, r.Scheme)
if err != nil {
r.Log.Error(err, "Set PVC ControllerReference")
}
return pvc
}

For your reference:

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestion @MortalHappiness,
I have changed the code to perform label setting in golang functions instead of yaml files.

Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

@ianshen1104 Thank you for your contributions. :-)

Comment on lines -1 to -17
#
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

Copy link
Member

Choose a reason for hiding this comment

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

hi @ianshen1104
The yaml file also needs to retain the Apache license description.
What is the reason for deletion?

Comment on lines -1 to -16
#
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
Copy link
Member

Choose a reason for hiding this comment

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

The yaml file also needs to retain the Apache license description.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants