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

Init observe func #57

Merged
merged 1 commit into from
May 20, 2022

Conversation

fahedouch
Copy link
Collaborator

@fahedouch fahedouch commented Apr 29, 2022

Signed-off-by: Fahed DORGAA fahed.dorgaa@gmail.com

Description of your changes

Fixes # observe func WIP; ObserveAndDelete RunPolicy

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

@fahedouch fahedouch marked this pull request as draft April 29, 2022 16:47
@fahedouch fahedouch marked this pull request as ready for review May 10, 2022 14:30
@fahedouch fahedouch changed the title init observe func [WIP] init observe func May 10, 2022
internal/controller/ansibleRun/ansibleRun.go Outdated Show resolved Hide resolved
internal/controller/ansibleRun/ansibleRun.go Outdated Show resolved Hide resolved
internal/controller/ansibleRun/ansibleRun.go Show resolved Hide resolved
internal/controller/ansibleRun/ansibleRun.go Outdated Show resolved Hide resolved
@fahedouch fahedouch changed the title [WIP] init observe func Init observe func May 16, 2022
@fahedouch fahedouch force-pushed the init-observe-func branch 2 times, most recently from d8f5603 to 1aed41a Compare May 16, 2022 17:23
@morningspace
Copy link
Collaborator

@fahedouch Thanks for your continued effort on this PR! I believe we are very close to merge it. Could you please check the last few comments I appended above:

  • It looks apiVersion was deleted for some reason, not sure if this is intentional.
  • GetPolicyRun/SetPolicyRun seems not being used.

Lastly, a couple of CI pipeline jobs failed, could you please take look? Thanks again!

go.mod Outdated Show resolved Hide resolved
package/crds/ansible.crossplane.io_providerconfigs.yaml Outdated Show resolved Hide resolved
package/crds/ansible.crossplane.io_ansibleruns.yaml Outdated Show resolved Hide resolved
internal/controller/ansibleRun/ansibleRun.go Outdated Show resolved Hide resolved
name: gcpbucket
spec:
forProvider:
# PlaybookSet default to using a remote source - like playbookSet-remote.yaml.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid the possible confusion to users, I think the comments here may need to be revised as the notion of PlaybookSet is something we previously used and has been replaced by AnsibleRun. Same applies to ansibleRun-inline.yaml.

Another thing I just realized is that the current AnsibleRun CR uses some spec fields that do not align with the design such as source and module, see the sample snippets at https://github.com/multicloudlab/crossplane-provider-ansible/blob/main/docs/design.md#inline where we actually do not have source and module, but use playbookInline instead.

Signed-off-by: Fahed DORGAA <fahed.dorgaa@gmail.com>
Copy link
Collaborator

@morningspace morningspace left a comment

Choose a reason for hiding this comment

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

Did the final round of review, it looks perfect! BIG THANKS @fahedouch 👍

@ibm-ci-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fahedouch, morningspace
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@morningspace morningspace merged commit 20ba32d into crossplane-contrib:main May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants