-
Notifications
You must be signed in to change notification settings - Fork 198
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
Konveyor and OLM add-ons #711
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freschri I added some tactical feedback, however on a more strategic items I have a few questions:
-
https://github.com/konveyor/konveyor.github.io does not appear to be production ready. I don't observe any releases at all and the versioning schema they apply is in the alpha range. In addition the repo does not appear to be actively maintained. Most contributions happened last year and I don't observe active development on it. We will only take actively maintained popular OSS projects here, so it appears that this addon may be best served in your personal github repo for now.
If you have access to the contributors and/or partner sponsoring the OSS then it may be best to move to their repo and we can include some doc updates on it. This PR can be split to just keep the OLM part which is indeed an enabler. -
OLM being non-helm addon : how do you envision maintenance of this add-on, e.g. version upgrades and testing against latest EKS versions? Do I have your commitment on ownership of this part?
@@ -0,0 +1,39 @@ | |||
kind: Tackle | |||
apiVersion: tackle.konveyor.io/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1alpha1 makes me think this product is not production ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
I'm a maintainer of Konveyor and can provide some more context of Konveyor's status.
Konveyor is an actively developed and maintained CNCF sandbox project that is the upstream for Red Hat's Migration Toolkit for Applications .
The code is in production at multiple large organizations being used to aid modernization engagements of enterprise java applications to Kubernetes. The code that makes up Konveyor began ~10 years ago on the Analysis side with the latest version of the platform being developed ~2 years ago, the solution was called Tackle2.
After being accepted into CNCF, there has been an effort to refocus Konveyor onto a Unified vision of bringing the functionality that used to be in separate projects together into a common solution which is Konveyor.
As part of this refocus, we decided to release Konveyor as a "v0.1" as opposed to continuing with the Tackle 2.1+ scheme that was in place prior.
It is possible for the API surface to change as we are working with the community to shape what the future will be. In regard to the API surface for the operator CR, we are careful to keep upgrades working and stable and have automated tests and QE resources actively helping to ensure stability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @jwmatthews, thank you for your feedback, it surely helps. Understand the comment related to the CR API version and upgrades. It is not as intuitive but it surely matches the saying "there is nothing more permanent than temporary" when it comes versioning.
I assume we should change the source references to https://github.com/konveyor/tackle2-operator, please confirm.
Is https://konveyor.github.io/ still the right documentation entry point? I saw a reference to tackle document here pointing to what appears to be the right page: https://konveyor.github.io/tackle. This url, however, is not functioning.
Understand that doc maintenance may not be the top priority, I was a lot more interesting in dev activities, which you confirmed. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, https://konveyor.github.io/ is the right documentation entry point, we are bringing back usage of this as the central spot for developers to contribute to.
The source code for the Konveyor operator is located at: https://github.com/konveyor/tackle2-operator
If you want to reference source code, use: https://github.com/konveyor/tackle2-operator
If you want to reference documentation use: https://konveyor.github.io/
As to https://konveyor.github.io/tackle, that is an older link and not intended.
lib/addons/olm/index.ts
Outdated
const resources: Construct[] = []; | ||
this.manifestUrls!.map((manifestUrl, urlIndex) => { | ||
/* eslint-disable */ | ||
const request = require('sync-request'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loads the sync-request library in a cycle redefining the function. Can you move it out of the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is bad, addressed.
const manifestDeployment: ManifestDeployment = { | ||
name: "olmIdx"+urlIndex+docIndex, | ||
namespace: "olm", | ||
manifest: [manifest], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why manifest is an array here? should be an actual doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addManifest ends up instantiating KubernetesManifest and it looks to me "manifest" is an array type in the param list, as per here: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_eks.KubernetesManifest.html#construct-props ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nm, I think we should do a better job on reflecting the types.
lib/addons/olm/index.ts
Outdated
values: {} | ||
}; | ||
|
||
const kubectlProvider = new KubectlProvider(clusterInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no need to instantiate kubectlProvider in a loop. let's move out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,17 @@ | |||
# Konveyor Amazon EKS Blueprints AddOn | |||
|
|||
This add-on installs [Konveyor](https://konveyor.github.io/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please elaborate on the use cases answering the question of why the customer would use this addon, expanding on scenarios where this product is applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this point to a different doc portal for tackle2 operator? The documentation page here is misleading and calls out the unmaintained repo as the source which is confusing (to me and I assume the target audience).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://konveyor.github.io/ is good to use for Konveyor documentation. The repo was stale and needed attention, hence we updated it ~2 weeks ago and are working to revamp this as the central documentation for Konveyor going forward.
lib/addons/olm/index.ts
Outdated
resources.push(...batchResources); | ||
}); | ||
|
||
return Promise.resolve(resources[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not this be last resource as opposed to 0? If it is first, then it won't wait for the rest of the resources to apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Hello, the repo above is not representative of Konveyor. Konveyor is actively being developed and maintained. That is one of the upstream documentation repos we created to have a single consolidated spot for documentation, we needed to pause that effort and switched to documenting areas in their respective repositories. We are beginning to get back to the consolidated doc effort, but I wouldn't consider it representative to gauge activity on Konveyor. Konveyor releases as an Operator, as such the first place to look would be the Operator repo which is at: The Konveyor operator deploys the components below:
The majority of development activity in Konveyor is centered on the Hub + UI.
|
docs/addons/konveyor.md
Outdated
- [EBS CSI Driver Amazon EKS add-on](https://aws-quickstart.github.io/cdk-eks-blueprints/addons/ebs-csi-driver/) (_EbsCsiDriverAddOn_), to provide Persistent Volumes (PVs) fulfilling Konveyor's Persistent Volume Claims (PVCs) | ||
- a subdomain for the Konveyor application | ||
- a certificate for the subdomain, made available by the either _CreateCertificateProvider_ or _ImportCertificateProvider_, to be assigned to the load balancer | ||
- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra white space for outline to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress, a couple of mostly minor notes.
@@ -0,0 +1,17 @@ | |||
# Konveyor Amazon EKS Blueprints AddOn | |||
|
|||
This add-on installs [Konveyor](https://konveyor.github.io/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this point to a different doc portal for tackle2 operator? The documentation page here is misleading and calls out the unmaintained repo as the source which is confusing (to me and I assume the target audience).
@@ -0,0 +1,39 @@ | |||
kind: Tackle | |||
apiVersion: tackle.konveyor.io/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @jwmatthews, thank you for your feedback, it surely helps. Understand the comment related to the CR API version and upgrades. It is not as intuitive but it surely matches the saying "there is nothing more permanent than temporary" when it comes versioning.
I assume we should change the source references to https://github.com/konveyor/tackle2-operator, please confirm.
Is https://konveyor.github.io/ still the right documentation entry point? I saw a reference to tackle document here pointing to what appears to be the right page: https://konveyor.github.io/tackle. This url, however, is not functioning.
Understand that doc maintenance may not be the top priority, I was a lot more interesting in dev activities, which you confirmed. Thank you.
I am closing this PR, following recommendation from Mikhail, requested Claranet availability to host the 2 addons, they confirmed: |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.