Closed roeiK-wix closed 2 years ago
Could we avoid merge commits and prefer rebasing instead?
During the implementation, we found that the old and now the new dry flag are just working inside the method section.
Can you clarify this? I think the dry flag is applicable to the ssh, right?
Could we avoid merge commits and prefer rebasing instead?
Yes, in the next contribution we will be much more careful, we won't merge, only rebase.
During the implementation, we found that the old and now the new dry flag are just working inside the method section.
Can you clarify this? I think the dry flag is applicable to the ssh, right?
So, I checked again, and the dry flag is working in all of the stages (include in the ssh) but it doesn't work in the controls. So, if I have a probe in the control it will be executed. I don't know if it's bad or not but it's something we should know about.
At the moment, the Build, Test, and Lint / lint (pull_request) check is continually failing
To resolve this, you can make use of the Makefile
by following the Develop and Formatting and Linting steps in the chaostoolkit-lib README.md
before committing any new changes
At the moment, the Build, Test, and Lint / lint (pull_request) check is continually failing To resolve this, you can make use of the
Makefile
by following the Develop and Formatting and Linting steps in the chaostoolkit-libREADME.md
before committing any new changes
Done, please approve @CharlieMoon37
Hi @roeiK-wix, It's looking like we are very nearly there ... I hope the changes that we have suggested make sense to you and that you understand where we were coming from. At the moment, I cannot approve the PR as @Lawouach still has some requested changes that have not been resolved. I think we will have to wait until tomorrow when they are back online so they can have a look and either re-request changes or decided that they have been resolved. At the moment though, I am unable to approve - but from my perspective, things are looking good :) Sorry to hold you back
(I'll have a look at the PR for the chaostoolkit now)
Hi @roeiK-wix, It's looking like we are very nearly there ... I hope the changes that we have suggested make sense to you and that you understand where we were coming from. At the moment, I cannot approve the PR as @Lawouach still has some requested changes that have not been resolved. I think we will have to wait until tomorrow when they are back online so they can have a look and either re-request changes or decided that they have been resolved. At the moment though, I am unable to approve - but from my perspective, things are looking good :) Sorry to hold you back
(I'll have a look at the PR for the chaostoolkit now)
Thanks for your comments it helped me to fix the problems quicker. I understand completely what you asked, this is what code quality looks like. I didn't bother by doing those changes, and I hope that in the future ill know how to contribute with fewer mistakes :)
Hey, we finished refactoring the dry flag according to what we agreed on. Flag options: --dry=actions - Not executing any actions in the method and rollback sections. --dry=probes - Not executing any probes in the method and rollback sections. --dry=activities - Not executing any actions and probes in the method and rollback sections. (the same as the old dry flag) --dry=pause - Not executing any pause. During the implementation, we found that the old and now the new dry flag are just working inside the method section. But in all the other sections such as the steady-state and in the controls, the activities are still working as normal (meaning no dry) We wanted to know if this is the logic you meant to when you thought about adding a dry flag? Also, we found that the probes aren't returning any value, (as expected to a function that doesn't been called) so it always returns None and so it's always false tolerance.
In this new implementation, we kept the old logic and added the new logic with actionless, probeless, and pauseless.
Also, review this PR -> chaostoolkit/chaostoolkit#220
We didn't add any tests for this new logic because we wanted you to first review this PR and then we will add the tests. Thanks. @Lawouach