rajatjindal / krew-release-bot

bot to bump version of plugin in krew-index on new releases
Apache License 2.0
46 stars 16 forks source link

Action fails if template is on line with `-` #25

Closed corneliusweig closed 4 years ago

corneliusweig commented 4 years ago

I just tried the GitHub Action here https://github.com/corneliusweig/rakkess/runs/409914601?check_suite_focus=true.

It looks like the action failed, because the addURIAndSha appears on the same line as -:

platforms:
    - {{ addURIAndSha "https://github.com/corneliusweig/rakkess/releases/download/{{ .TagName }}/access-matrix-amd64-linux.tar.gz" .TagName }}
      bin: access-matrix
      files:
        - from: ./LICENSE
          to: .
        - from: ./access-matrix-amd64-linux
          to: access-matrix
      selector:
        matchLabels:
          os: linux
          arch: amd64

This is the log for the run:

time="2020-01-26T23:47:04Z" level=info msg="using template file \"/github/workspace/hack/access-matrix.yaml\"" time="2020-01-26T23:47:04Z" level=info msg="getting sha256 for https://github.com/corneliusweig/rakkess/releases/download/v0.4.3/access-matrix-amd64-linux.tar.gz" time="2020-01-26T23:47:04Z" level=info msg="downloaded file /tmp/004308497/1580082424" time="2020-01-26T23:47:04Z" level=info msg="getting sha256 for https://github.com/corneliusweig/rakkess/releases/download/v0.4.3/access-matrix-amd64-darwin.tar.gz" time="2020-01-26T23:47:05Z" level=info msg="downloaded file /tmp/853989436/1580082424" time="2020-01-26T23:47:05Z" level=info msg="getting sha256 for https://github.com/corneliusweig/rakkess/releases/download/v0.4.3/access-matrix-amd64-windows.zip" time="2020-01-26T23:47:05Z" level=info msg="downloaded file /tmp/468274795/1580082425" time="2020-01-26T23:47:05Z" level=fatal msg="error converting YAML to JSON: yaml: line 8: did not find expected '-' indicator"

Is that possible? Or maybe I made some other blunder..

rajatjindal commented 4 years ago

yes that might be the issue. i will have to check. the other issue to test templating will help

corneliusweig commented 4 years ago

I got the same error again (https://github.com/corneliusweig/rakkess/runs/409930876?check_suite_focus=true) after moving the template block downwards. Maybe you can spot a problem with my yaml file? https://github.com/corneliusweig/rakkess/blob/c35ffd4fe9941fbded693a5c106b6bbced1b6cb4/hack/access-matrix.yaml#L9

rajatjindal commented 4 years ago

I understand now what is the issue. its with the assumption I had with respect to indentation. Look at uri/sha256 below. its a bug. I will try to fix this today. thanks.

apiVersion: krew.googlecontainertools.github.com/v1alpha2
kind: Plugin
metadata:
  name: access-matrix
spec:
  version: v0.4.3
  platforms:
    - bin: access-matrix
      uri: https://github.com/corneliusweig/rakkess/releases/download/v0.4.3/access-matrix-amd64-linux.tar.gz
    sha256: f455e202e932578a43e11813c5e19b9fd82ad758ea5ec119db3aa5f686948bd3
      files:
        - from: ./LICENSE
          to: .
        - from: ./access-matrix-amd64-linux
          to: access-matrix
      selector:
        matchLabels:
          os: linux
          arch: amd64
    - bin: access-matrix
      files:
        - from: ./LICENSE
          to: .
        - from: ./access-matrix-amd64-darwin
          to: access-matrix
      selector:
        matchLabels:
          os: darwin
          arch: amd64
    - bin: access-matrix.exe
      files:
        - from: ./LICENSE
          to: .
        - from: ./access-matrix-amd64-windows.exe
          to: access-matrix.exe
      selector:
        matchLabels:
          os: windows
          arch: amd64
  shortDescription: Show an RBAC access matrix for server resources
  homepage: https://github.com/corneliusweig/rakkess
  caveats: |
      Usage:
        kubectl access-matrix
        kubectl access-matrix for pods
  description: |
      Show an access matrix for server resources
      This plugin retrieves the full list of server resources, checks access for
      the current user with the given verbs, and prints the result as a matrix.
      This complements the usual "kubectl auth can-i" command, which works for
      a single resource and a single verb. For example:
       $ kubectl access-matrix
      It also supports a mode which prints all subjects with access to a given
      resource (needs read access to Roles and ClusterRoles). For example:
       $ kubectl access-matrix for configmap
corneliusweig commented 4 years ago

I see. I think a clean solution could be to substitute the .TagName via templating, but then read the rendered manifest via yaml ummarshalling. Then you can set the sha256 on the go object and marshal the complete go object to yaml again. WDYT?

rajatjindal commented 4 years ago

I had considered that approach when I started the work on it, one downside of using that approach is that when we convert it back to yaml, we get fields sorted alphabetically, which is not very readable:

observe how version went to the last line while having it right at starting of spec is what we desired.

apiVersion: krew.googlecontainertools.github.com/v1alpha2
kind: Plugin
metadata:
  creationTimestamp: null
  name: whoami
spec:
  caveats: "This plugin has only been tested with RBAC token, ServiceAccount token,
    and BasicAuth. \n\nIt will be great if we can get volunteers to test it with other
    Auth providers.\n\nRead the documentation at:\n  https://github.com/rajatjindal/kubectl-whoami\n"
  description: |
    This plugin show the subject that's currently authenticated as.
  homepage: https://github.com/rajatjindal/kubectl-whoami
  platforms:
  - bin: kubectl-whoami
    files:
    - from: '*'
      to: .
    selector:
      matchLabels:
        arch: amd64
        os: darwin
    uri: https://github.com/rajatjindal/kubectl-whoami/releases/download/v0.0.29/kubectl-whoami_v0.0.29_darwin_amd64.tar.gz
  - bin: kubectl-whoami
    files:
    - from: '*'
      to: .
    selector:
      matchLabels:
        arch: amd64
        os: linux
  shortDescription: Show the subject that's currently authenticated as.
  version: v0.0.29
corneliusweig commented 4 years ago

Well, I think sorted alphabetically is not so bad. This ensures that the every manifest will look alike. Not just the revisions of a particular manifest. Besides, plugin manifests are not even meant to be read by users, so I think this is fine.

On the other hand, it may be annoying for users to see their manifests reorderd.

This is your call. Whatever you think is more appropriate.

rajatjindal commented 4 years ago

I do not have strong opinion about this. I am open as long as you and @ahmetb give this a thumbs up.

Thanks

ahmetb commented 4 years ago

I guessed this would happen, but we can list it as a limitation and move on. Not sure if worth fixing if we control all examples.

rajatjindal commented 4 years ago

hey Ahmet, just so to make sure I understand u correctly, are u suggesting we can close this issue without fixing?

i am writing a sub-command of action to run templating without having to run the complete action, so that will help us catch the issue in a slightly more graceful manner.

ahmetb commented 4 years ago

You can keep it open as a backlog but IMO it’s not urgent since the workaround is pretty easy.

corneliusweig commented 4 years ago

i am writing a sub-command of action to run templating without having to run the complete action, so that will help us catch the issue in a slightly more graceful manner.

:+1: for this.

As long as it's well documented, there's probably no urgent need to fix this.

rajatjindal commented 4 years ago

opened PR: #28

rajatjindal commented 4 years ago

fixed with v0.0.33. we have added function indent that can be used if user is not using default indentation of 2 spaces.

{{ addURIAndSha "https://github.com/corneliusweig/rakkess/releases/download/{{ .TagName }}/access-matrix-amd64-linux.tar.gz" .TagName | indent 6 }}