Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Kogito-2195] Kogito examples with ansible automation #258
base: main
Are you sure you want to change the base?
[Kogito-2195] Kogito examples with ansible automation #258
Changes from 1 commit
1e07b99
654af19
f3971c0
4dee7f9
7c027cd
73d1f78
5eb0cda
48847f8
85b42af
a0cc19a
d78b179
b188a30
d157a97
8eb988a
2df8866
baba573
b77023d
31ac8dd
cd77312
cd487d7
2c80cf1
fa52112
be88c25
35f6e3a
da2fcb6
e76eb8e
260aa18
5489c7b
1fae857
78f4018
18c5e10
da06eff
bb7c906
4df8df3
f904f2c
0f04340
ebd0d10
87eb4ff
7762734
fc05cff
7138182
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 make this optional. User may want to use his already CRC deployed cluster. It is probably not good to force user to remove all his previous CRC setup.
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.
Ok
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 provide version of Ubuntu and Debian on which you have tested these scripts.
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.
Ok
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 provide setup also for Fedora.
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.
I'll create two sections in the doc since is all for fedora and only the lib virt installation is the first step on ubuntu
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.
Isn't this done by default?
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.
No, is disabled by default to save time, but to read the current user infos is needed
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.
Do we really need to explicitly run this?
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.
No, was just to describe the version used, we could use a placeholder and print the version used instead to updated this on every update
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 is this value hardcoded?
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.
Because is used in the path and you discover the version only when the crc is downloaded and unpacked, or readed from the lates json info
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.
I've writed a version to use the latest kogito tag, but the 0.10.1 doesn't work because the cli and the operator aren't aligned (kogito 0.10.1 - kogito-cloud-operator 0.10.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.
mmm I expect this could happen in the future too so is it possible to add a
kogito-operator
variable to support also this use case?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 do not use this value which can probably change on different machine etc. It would be much better to use "virt" or "virt_net" modules to obtain info about IP address.
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.
Ok
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.
Can you please specify variable for this with default value to make it configurable.
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.
Ok
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.
Again please create variable for this temporary destination with default value. Or you can also consider to use "tempfile" module. https://docs.ansible.com/ansible/latest/modules/tempfile_module.html#tempfile-module
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.
Ok
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.
Isn't DNS reconfigured by CRC itself.
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.
I have to check with the latest version, because with old version of crc without this manual addition on /etc/hosts the cluster was unreachable
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.
What is the reason for gather_facts here? As far as I know Ansible will gather facts automatically at start of the playbook.
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 use "apt" module instead of shell. https://docs.ansible.com/ansible/latest/modules/apt_module.html#apt-module
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.
Ok
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 check that os family is supported using assert module instead of silently skipping installation of libvirt libraries.
https://docs.ansible.com/ansible/latest/modules/assert_module.html#assert-module
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.
Ok