Skip to content

update crossplane runtime#369

Merged
d-honeybadger merged 4 commits intomainfrom
dkomsa/update-crossplane-runtime
May 17, 2025
Merged

update crossplane runtime#369
d-honeybadger merged 4 commits intomainfrom
dkomsa/update-crossplane-runtime

Conversation

@d-honeybadger
Copy link
Copy Markdown
Collaborator

@d-honeybadger d-honeybadger commented May 6, 2025

Description of your changes

Updates crossplane-runtime as well as k8s packages and go (so that k8s + crossplane are compatible).
Used this PR as inspiration: crossplane-contrib/provider-aws#2165

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

TODO: run locally and verify major functionality end-to-end

Signed-off-by: d-honeybadger <komsa.darya@gmail.com>
Signed-off-by: d-honeybadger <komsa.darya@gmail.com>
@d-honeybadger d-honeybadger force-pushed the dkomsa/update-crossplane-runtime branch from f45ef27 to 3a07e5c Compare May 6, 2025 03:44
Signed-off-by: d-honeybadger <komsa.darya@gmail.com>
@d-honeybadger d-honeybadger force-pushed the dkomsa/update-crossplane-runtime branch 2 times, most recently from 5db00d3 to 8b2a2ff Compare May 10, 2025 04:11
artifactsHistoryLimit = app.Flag("artifacts-history-limit", "Each attempt to run the playbook/role generates a set of artifacts on disk. This settings limits how many of these to keep.").Default("10").Int()
pollStateMetricInterval = app.Flag("poll-state-metric", "State metric recording interval").Default("5s").Duration()
replicasCount = app.Flag("replicas", "Amount of replicas configured for the provider. When using more than 1 replica, reconciles will be sharded across them based on a modular hash.").Default("1").Int()
replicasCount = app.Flag("replicas", "Amount of replicas configured for the provider. When using more than 1 replica, reconciles will be sharded across them based on a modular hash.").Default("1").Uint32()
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

golangci-lint was complaining about int-uint32 conversion later in the code, which has the potential to overflow. I
changed all intermediate and this one "source" var relevant to this case to be uint32 in the first place. They are all things like replaca counts, number of shards and shard indices and cannot be negative.

"github.com/crossplane/crossplane-runtime/pkg/statemetrics"
"github.com/google/uuid"
"github.com/spf13/afero"
"gopkg.in/yaml.v2"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Replaced this one with "sigs.k8s.io/yaml":
Linter complained that the struct that was getting yaml.Marshaled didn't have yaml struct tags. It only had json tags.
I saw no reason to add yaml tags given that sigs.k8s.io/yaml package exists, which works with (only) json tags and is just better for working with simple yaml (specifically, the subset of yaml that is json-representable).

…ng settings, fix int -> uint32 potential overflow, fix yaml package requiring yaml struct tags rather than json

Signed-off-by: dkomsa <dkomsa@digitalocean.com>
@d-honeybadger d-honeybadger force-pushed the dkomsa/update-crossplane-runtime branch from 8b2a2ff to 58d4d24 Compare May 10, 2025 04:35
@d-honeybadger d-honeybadger marked this pull request as ready for review May 10, 2025 04:38
Copy link
Copy Markdown
Collaborator

@fahedouch fahedouch left a comment

Choose a reason for hiding this comment

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

Thanks

GO_VERSION: '1.21'
GOLANGCI_VERSION: 'v1.56.2'
GO_VERSION: '1.23'
GOLANGCI_VERSION: 'v1.64.2'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

need to prepare the migration to v2, this can be in a follow-up PR

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, there seem to be so many changes that I dreaded updating to v2 in this PR, but needs to get done someday

}

func (e *external) Disconnect(ctx context.Context) error {
// Unimplemented, required by newer versions of crossplane-runtime
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

need to reflect this in the doc schemes but this can be done in a follow up PR too!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Umm by schemes do you mean the diagrams under the docs/ dir? Or something else? Do you have editable sources for those images?

@fahedouch fahedouch added this to the v0.7.0 milestone May 12, 2025
@fahedouch
Copy link
Copy Markdown
Collaborator

thanks @d-honeybadger

TODO: run locally and verify major functionality end-to-end

you did some local tests ?

@d-honeybadger
Copy link
Copy Markdown
Collaborator Author

Thanks for the review @fahedouch!
Finally got my local setup working (I used to do it in a work cluster before, had to figure out a dev flow with deploying provider-ansible to a kind cluster).
And got a successful run (as well as failed ones while I was figuring things out 😄 )

apiVersion: ansible.crossplane.io/v1alpha1
kind: AnsibleRun
metadata:
  annotations:
    ansible.crossplane.io/runPolicy: ObserveAndDelete
    crossplane.io/external-name: test
    kubectl.kubernetes.io/last-applied-configuration: '{"inventoryInline":"159.203.23.175\n","inventories":null,"executableInventory":false,"playbookInline":"---\n-
      hosts: all\n  tasks:\n    - name: create file\n      ansible.builtin.file:\n        path:
      /tmp/foo.bar\n        owner: root\n        group: root\n        mode: ''0644''\n        state:
      touch\n\n    - name: debug\n      ansible.builtin.shell: ls -l /tmp/foo.bar\n","roles":null,"vars":{"ANSIBLE_HOST_KEY_CHECKING":"False","ansible_ssh_private_key_file":"test-key","ansible_user":"root"}}'
  creationTimestamp: "2025-05-17T04:40:47Z"
  finalizers:
  - finalizer.managedresource.crossplane.io
  generation: 6
  name: test
  resourceVersion: "6605"
  uid: 14468005-a708-4635-8b4a-2dec18c749a2
spec:
  deletionPolicy: Orphan
  forProvider:
    executableInventory: false
    inventoryInline: |
      <ip here>
    playbookInline: |
      ---
      - hosts: all
        tasks:
          - name: create file
            ansible.builtin.file:
              path: /tmp/foo.bar
              owner: root
              group: root
              mode: '0644'
              state: touch

          - name: debug
            ansible.builtin.shell: ls -l /tmp/foo.bar
    vars:
      ANSIBLE_HOST_KEY_CHECKING: "False"
      ansible_ssh_private_key_file: test-key
      ansible_user: root
  managementPolicies:
  - '*'
  providerConfigRef:
    name: test
status:
  atProvider: {}
  conditions:
  - lastTransitionTime: "2025-05-17T05:05:16Z"
    reason: ReconcileSuccess
    status: "True"
    type: Synced
  - lastTransitionTime: "2025-05-17T05:05:16Z"
    reason: Available
    status: "True"
    type: Ready

So, merging this one

@d-honeybadger d-honeybadger merged commit dcec69c into main May 17, 2025
7 checks passed
@d-honeybadger d-honeybadger deleted the dkomsa/update-crossplane-runtime branch May 17, 2025 05:14
@fahedouch
Copy link
Copy Markdown
Collaborator

thanks @d-honeybadger, So we are ready for a release ^^

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.

2 participants