Skip to content
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

chore(helm): Use template comments for the chart license header #23726

Merged
merged 1 commit into from
May 1, 2023

Conversation

dnskr
Copy link
Contributor

@dnskr dnskr commented Apr 18, 2023

SUMMARY

The PR changes comment type for license headers in the Helm chart template files.
The PR does not change any functionality of the chart.

According to Comments (YAML Comments vs. Template Comments):

YAML comments may be used when it is useful for Helm users to (possibly) see the comments during debugging

Since there is no value in seeing license header during templates debug, it's a good idea to use Template comments instead of YAML comments for license headers.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

$ helm template superset helm/superset
---
# Source: superset/templates/service.yaml
#
# 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.
#
apiVersion: v1
kind: Service
metadata:
  name: superset
  namespace: default
  labels:
    app: superset
    chart: superset-0.9.2
    release: superset
    heritage: Helm
spec:
  type: ClusterIP
  ports:
    - port: 8088
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app: superset
    release: superset

After

$ helm template superset helm/superset
---
# Source: superset/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  name: superset
  namespace: default
  labels:
    app: superset
    chart: superset-0.9.2
    release: superset
    heritage: Helm
spec:
  type: ClusterIP
  ports:
    - port: 8088
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app: superset
    release: superset

TESTING INSTRUCTIONS

Check that the following command doesn't print any license headers.

helm template superset helm/superset

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dnskr
Copy link
Contributor Author

dnskr commented Apr 18, 2023

✖︎ superset => (version: "0.9.2", path: "helm/superset") > chart version not ok. Needs a version bump!

@craig-rueda should I bump the version if there are no functional changes?

@craig-rueda
Copy link
Member

Yep, looks like the linter is complaining

@dnskr
Copy link
Contributor Author

dnskr commented Apr 29, 2023

@craig-rueda Could you please have a look again?
Current helm chart version 0.9.3 is the same as PR version, i.e. it is possible to merge but not release the chart.

Copy link

@ankit-gautam23 ankit-gautam23 left a comment

Choose a reason for hiding this comment

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

LGTM

@craig-rueda
Copy link
Member

Looks fine as long as the license check step is happy. Seems like it skipped for this PR for some reason. Any ideas why?

@dnskr
Copy link
Contributor Author

dnskr commented May 1, 2023

Looks fine as long as the license check step is happy. Seems like it skipped for this PR for some reason. Any ideas why?

@craig-rueda There are two license checks: the first passed, the second was skipped. But as I can see the second check was skipped in other PRs as well.

Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

LGTM

@craig-rueda craig-rueda merged commit a994145 into apache:master May 1, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants