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

Adding new manifests and fixing old ones because of a lot of issues. #18

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

SultankaReal
Copy link

Adding new manifests with MySQLDatabase, MySQLUser, PostgresqlDatabase, PostgresqlUser.
Fixing old ones because of a lot of issues.
All manifests have been checked and tested.

SultankaReal and others added 2 commits March 6, 2023 12:06
…e, PostgresqlUser.

Fixing old ones because of a lot of issues.
All manifests have been checked and tested.
…-examples

Adding new manifests with MySQLDatabase, MySQLUser, PostgresqlDatabase, PostgresqlUser. Fixing old ones because of a lot of issues. All manifests have been checked and tested.
Copy link
Contributor

@vaspahomov vaspahomov left a comment

Choose a reason for hiding this comment

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

Hi, Nursultan! Thanks for you contribution. I've leave some comments, would you mind fixing it please?

spec:
forProvider:
name: example-registry
folderIdRef:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave refs here and everywhere. It's more correct way to make resource references with crossplane

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.
But sometimes users face with the issue when they use Refs (folderIdRef, networkIdRef, etc...).

For example, when they apply the manifest, they see something like this in 'kubectl describe':
**CannotResolveResourceReferences 7s (x2 over 7s) managed/vpc.yandex-cloud.jet.crossplane.io/v1alpha1, kind=network cannot resolve references: mg.Spec.ForProvider.FolderID: cannot get referenced resource: Folder.resourcemanager.yandex-cloud.jet.crossplane.io "b1g67uo02ppktpkktrpg" not found

That's why I leave comments for additional information to users that they can not only use Refs.

folderIdRef:
name: example-folder
folderId: <your-folder-id>
role: admin # укажи требуемую роль
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use english in comments

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@@ -1,11 +1,10 @@
apiVersion: container.yandex-cloud.jet.crossplane.io/v1alpha1
kind: Registry
metadata:
name: example-registry
name: registry-1994
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use example in names

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

data:
- "10.1.0.1"
- '<ip-address>' # тут должен быть ip-адрес
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not valid filed value. It's beter to use valid values, and add descriptions in comments

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

folderId: <your-folder-id>
name: yc-courses-1999 # укажи имя своей dns-зоны
public: true
zone: pushka.ga. # укажи свою dns-зону
Copy link
Contributor

Choose a reason for hiding this comment

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

I beleve that not all users will be allowed to create pushka.ga zone.
And example.com was made for testing purpose - https://www.iana.org/domains/reserved

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. I get it.

@@ -1,13 +1,11 @@
apiVersion: iam.yandex-cloud.jet.crossplane.io/v1alpha1
kind: FolderIAMMember
metadata:
name: crossplane-preprod
name: lala19
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use smth like folder-iam-member-example

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

clusterConfig:
- version: "4.2"
- version: "4.4" # required either [4.4 4.2 4.0 3.6 5.0-enterprise 4.4-enterprise 5.0]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to provide link on some site where supported versions described. It will always be updated without commits in repo

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, I didn't find the appropriate link for it. That's why I deleted these comments.

@nekr0z
Copy link
Member

nekr0z commented Sep 3, 2024

Examples have been updated since; do you think this PR is still valuable?

nekr0z pushed a commit that referenced this pull request Sep 12, 2024
…listeners to master

* commit '96e65a48b89c3c9a6eacf0118ae03ee01a49a348':
  Remove some old broken tests
  Reduce code duplication
  Update version
  Group listeners by IP and protocol
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