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: add caching client #129

Closed

Conversation

matthisholleville
Copy link
Contributor

Closes #113

📑 Description

This pull request is intended to implement the remote cache feature in the operator. This feature enables users to utilize a remote cache via an S3 bucket.

Modifications have been made to the client's structure in order to separate the different client methods into multiple files. Additionally, an implementation example has been added to the README.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

ℹ Additional Information

Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
@matthisholleville matthisholleville requested review from a team as code owners May 28, 2023 05:29
@@ -199,7 +206,15 @@ func (r *K8sGPTReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
r.k8sGPTClients[k8sgptConfig.Name] = k8sgptClient
}

response, err := r.k8sGPTClients[k8sgptConfig.Name].ProcessAnalysis(deployment, k8sgptConfig)
if k8sgptConfig.Spec.RemoteCache != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be called once and only once right? Or do you think we should be updating the config pre-analysis? It might be a good thing I suppose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think it's necessary to call it in two cases: when the object is created and when it's modified. What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to call this on object creation, call it further up before the finalizer is created

README.md Show resolved Hide resolved
)
}
addRemoteCacheEnvVar("AWS_ACCESS_KEY_ID", config.Spec.RemoteCache.Credentials.AccessKeyID)
addRemoteCacheEnvVar("AWS_SECRET_ACCESS_KEY", config.Spec.RemoteCache.Credentials.SecretAccessKey)
Copy link
Member

Choose a reason for hiding this comment

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

I am confused by this and by the new CredentialsRef type,

This should:

// Check to see if that secret exists ( missing)
// Unwrap the secret literal values ( missing )
// Mount each value as an EnvVarSecret ( done )

Copy link
Contributor Author

@matthisholleville matthisholleville May 28, 2023

Choose a reason for hiding this comment

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

In my opinion, to be consistent with the rest of the code, it is necessary to check this: config.Spec.RemoteCache.Credentials != nil. As for the secret related to the OpenAI credentials, it seems to me that we are not checking the presence of the secret itself, but rather the configuration. Am I mistaken?

api/v1alpha1/k8sgpt_types.go Show resolved Hide resolved
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
@AlexsJones
Copy link
Member

LMK when to review again!

@mrueg
Copy link

mrueg commented Jun 14, 2023

just curious, will this change restore compatibility with later versions (newer than 0.3.4) of k8sgpt?

@AlexsJones
Copy link
Member

I have updated this PR here @matthisholleville #201

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.

[Feature]: Support remote caching through the K8sGPT CR
3 participants