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

feat(bump): update upjet, and deps #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

haarchri
Copy link
Contributor

@haarchri haarchri commented Aug 3, 2023

Description of your changes

implement latest changes from upjet
add management policies (crossplane/upjet#248)

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Signed-off-by: Christopher Haar <[email protected]>
@haarchri haarchri force-pushed the feature/bump-deps branch from 7217450 to d08da60 Compare August 3, 2023 18:05
@haarchri
Copy link
Contributor Author

haarchri commented Aug 3, 2023

i use the following WorkspaceStore config:

		WorkspaceStore: terraform.NewWorkspaceStore(log, terraform.WithFeatures(featureFlags)),

and the e2e uptests green:

--- PASS: kuttl (37.35s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/case (36.42s)
PASS
20:05:14 [ OK ] running automated tests

if i use the following WorkspaceStore config:

		WorkspaceStore: terraform.NewWorkspaceStore(log, terraform.WithDisableInit(len(*nativeProviderPath) != 0), terraform.WithProcessReportInterval(*pollInterval), terraform.WithFeatures(featureFlags)),

i get the follwoing error in managedResource:

apiVersion: v1
items:
- apiVersion: null.template.upbound.io/v1alpha1
  kind: Resource
  metadata:
    annotations:
      upjet.upbound.io/test: "true"
    creationTimestamp: "2023-08-03T17:58:02Z"
    generation: 1
    name: example
    resourceVersion: "801"
    uid: 041f86b8-c9eb-4b2f-a4b5-b486828a7784
  spec:
    deletionPolicy: Delete
    forProvider:
      triggers:
        example-trigger: example-value
    managementPolicies:
    - '*'
    providerConfigRef:
      name: default
  status:
    atProvider: {}
    conditions:
    - lastTransitionTime: "2023-08-03T17:58:03Z"
      message: |-
        observe failed: cannot run refresh: refresh failed: Inconsistent dependency lock file: The following dependency selections recorded in the lock file are inconsistent with the current configuration:
          - provider registry.terraform.io/hashicorp/null: required by this configuration but no version is selected

        To make the initial dependency selections that will initialize the dependency lock file, run:
          terraform init
      reason: ReconcileError
      status: "False"
      type: Synced
kind: List
metadata:
  resourceVersion: ""

@lsviben
Copy link

lsviben commented Aug 4, 2023

i use the following WorkspaceStore config:

		WorkspaceStore: terraform.NewWorkspaceStore(log, terraform.WithFeatures(featureFlags)),

and the e2e uptests green:

--- PASS: kuttl (37.35s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/case (36.42s)
PASS
20:05:14 [ OK ] running automated tests

if i use the following WorkspaceStore config:

		WorkspaceStore: terraform.NewWorkspaceStore(log, terraform.WithDisableInit(len(*nativeProviderPath) != 0), terraform.WithProcessReportInterval(*pollInterval), terraform.WithFeatures(featureFlags)),

i get the follwoing error in managedResource:

apiVersion: v1
items:
- apiVersion: null.template.upbound.io/v1alpha1
  kind: Resource
  metadata:
    annotations:
      upjet.upbound.io/test: "true"
    creationTimestamp: "2023-08-03T17:58:02Z"
    generation: 1
    name: example
    resourceVersion: "801"
    uid: 041f86b8-c9eb-4b2f-a4b5-b486828a7784
  spec:
    deletionPolicy: Delete
    forProvider:
      triggers:
        example-trigger: example-value
    managementPolicies:
    - '*'
    providerConfigRef:
      name: default
  status:
    atProvider: {}
    conditions:
    - lastTransitionTime: "2023-08-03T17:58:03Z"
      message: |-
        observe failed: cannot run refresh: refresh failed: Inconsistent dependency lock file: The following dependency selections recorded in the lock file are inconsistent with the current configuration:
          - provider registry.terraform.io/hashicorp/null: required by this configuration but no version is selected

        To make the initial dependency selections that will initialize the dependency lock file, run:
          terraform init
      reason: ReconcileError
      status: "False"
      type: Synced
kind: List
metadata:
  resourceVersion: ""

Seems somehow connected with adding the terraform.WithDisableInit(len(*nativeProviderPath) != 0) but I am not that familiar with that feature. Maybe @ulucinar can shead some light on it?

@ulucinar
Copy link
Collaborator

ulucinar commented Aug 4, 2023

Hi folks,
The template provider does not use the shared gRPC server and instead, relies on the Terraform CLI to manage the lifecycle of the native provider process. Thus, we cannot use terraform.WithDisableInit(true) with the template provider. This option causes the provider to skip running a terraform init during the workspace initialization, and hence the error observed by @haarchri. We can skip running a terraform init only with the shared gRPC implementation.

@haarchri
Copy link
Contributor Author

haarchri commented Aug 4, 2023

so then everything is fine with this PR - can i get a review @ulucinar ;)

@@ -37,11 +38,10 @@ func main() {
var (
app = kingpin.New(filepath.Base(os.Args[0]), "Terraform based Crossplane provider for Template").DefaultEnvars()
debug = app.Flag("debug", "Run with debug logging.").Short('d').Bool()
syncPeriod = app.Flag("sync", "Controller manager sync period such as 300ms, 1.5h, or 2h45m").Short('s').Default("1h").Duration()
syncInterval = app.Flag("sync", "Sync interval controls how often all resources will be double checked for drift.").Short('s').Default("1h").Duration()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @haarchri,
The sync interval has a rather specific meaning tied to the internals of the controller-runtime:
https://github.com/kubernetes-sigs/controller-runtime/blob/304027bcbe4b3f6d582180aec5759eb4db3f17fd/pkg/cache/cache.go#L146

So, I would personally prefer keeping the reference to the controller-runtime in the explanation. There's also a warning there:

Change this value only if you know what you are doing. Defaults to 10 hours if unset.

I remember us discussing the name of this parameter and I remember saying syncInterval is a better alternative than the syncPeriod. Looks like we have borrowed the name from the upstream configuration parameter we are overriding. I've also seen that the core Crossplane documents it in the way suggested in this PR.

Even though we do the renaming to be consistent in the ecosystem, I believe we had better continue mentioning the controller-runtime and warn users that this is rather an advanced parameter to tackle with:

Suggested change
syncInterval = app.Flag("sync", "Sync interval controls how often all resources will be double checked for drift.").Short('s').Default("1h").Duration()
syncInterval = app.Flag("sync", "Controller manager cache sync period such as 300ms, 1.5h, or 2h45m. Determines the minimum frequency at which watched resources are reconciled. Use with caution.").Short('s').Default("1h").Duration()

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.

3 participants