@blueorangutan package

@RodrigoDLopez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✔centos7 ✔debian. JID-1743

@blueorangutan package

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2040

the check is removed for everybody and not only for admin. Is this intentional? it does not seem right

Can normal users execute the migrate volume command? As far as I remember they should not be able to do so. Of course, with the dynamic roles, we removed the restriction on hard-coded roles, and the restrictions are executed on the fly according to the allowed/denied APIs configured for a given role. Therefore, if the user has access to the API, it should be able to execute it and migrate the volume as he/she wishes.

@PaulAngus @rhtyd can you consider this?

@DaanHoogland, @rhtyd, @PaulAngus
Hello guys, happy new year.

Is there something missing here? Everything seems to be ok with the patch. It is a good bug fix, it has no errors, and tests are passing.

@rhtyd @DaanHoogland @PaulAngus @harikrishna-patnala @weizhouapache

Hello everyone... can I please receive some reviews on this PR?
This tag check sounds unnecessary at this point and those changes were tested by me, but I need other collaborators to approve and test these changes. Or maybe explain to me why those changes are not relevant to our community...

@RodrigoDLopez in view of the functional discussion in #4283 I am ignoring this. That discussion should actually take place on the dev list but soit.

Hi @RodrigoDLopez sorry busy with the dot releases, I'll revisit this soon.

@blueorangutan package

Is this one ready for merge?

I did a highlevel check I hesitate with removing these checks, @sureshanaparti - do you want to add some ideas we recently discussed?

I did a highlevel check I hesitate with removing these checks, @sureshanaparti - do you want to add some ideas we recently discussed?

could you please explain me why?

@sureshanaparti can you share this ideas with me?

ping @sureshanaparti @harikrishna-patnala - I think you're working on something similar care to chime in here please

ping @rhtyd @GabrielBrascher @sureshanaparti @DaanHoogland @weizhouapache

Any update on this one?

@rhtyd @GabrielBrascher @sureshanaparti @weizhouapache @harikrishna-patnala , I understand there are some objections/alternatives planned but there is no reference here to a discussion on the topic. If there isn't one we should go ahead with this.

I'm working on this PR #5008 to address the migration of volumes linked with disk offering to make it more flexible and refactoring around service offering and disk offering.

The behaviour will be almost the same w.r.t this PR but with more flexibility using a configuration parameter, so I think we can close this PR. I'll also produce a document on this to discuss.

@RodrigoDLopez can you check out #5008 to see if it meets your requirements and to decide if you still want to go on with this, please?

@RodrigoDLopez @harikrishna-patnala please advise if this PR should be closed in favour of #5008

Ping @RodrigoDLopez @harikrishna-patnala

@DaanHoogland @nvazquez This PR has a good description, tests were reported, is quite simple, easy to test, has 4 approves and came first (almost 1 year ago), while #5008 is pretty big, has a lot of changes, none description and still is a draft.

I think we should run tests here and merge it, if it is alright; Then rebase #5008.

I don't see why this PR should be closed in favour of #5008. #4257 has a clear description and addresses a specific issue that can be easily tested, reproduced, and if necessary, reverted.

Additionally, last time I've checked #5008 it was without description and set as a draft; looking #5008 right now it hasn't changed much. Nothing against #5008, but I think that both PRs can live together and this one has been open for a long time.

To be honest, I am a bit worried with how we are handling some PRs from new contributors. It almost seems like some contributors have a hard time proposing, discussing, implementing, testing ... and then the PR is closed in favour of another work that introduces the same fix/behavior.

This is just an example. If we go this way it will start to be hard for new contributors to implement something as it can be put aside for months and then closed in favour of any PR.

I understand @RodrigoDLopez frustrations, some other examples that I've noticed are:

• Removes unnecessary validations #4283
• Prevents null pointers when doing consecutive VM migrates #4282

I'm not against this PR, but just to avoid redundant changes in two different PRs, I've asked to close this one. PR 5008 is taking more time, let's merge this and I'll rebase my PR accordingly. My apologies @RodrigoDLopez.

There are no smoketests even kicked when the PR was originally created, I only see one comment that was made yesterday asking for tests to be kicked. Unless we've 2xLGTM and smoketests we can't merge a PR, furthermore we can't simply merge any PR irrespective of whether it's a new or old contributor - they need to be not introducing any regression. I've not looked into the code or tested this, but this PR removes checks and can potentially cause regressions, and introduces a change in behaviour.

@harikrishna-patnala why was this merged without packaging or kicking smoketests? cc 4.16 RM @nvazquez

@GabrielBrascher usually, the RM would review all open PRs/issues and for PRs pay attention and try to engage and review/test PRs and merge them and I see our RM for 4.16 @nvazquez has made that attempt so I wouldn't say the contributor's PR didn't get any help or attention. In fact, we see a huge no. of old PRs where authors don't even respond to RMs, so it would be incorrect to assume that any contributors are not getting any help in general, but as people work in their "free time" we can't compel who helps and when.

New contributors should learn to build support, learn how to improve their code and communicate in a way to get their PRs merged. I would ask them to start by identify who the active contributors in the project are (usually the RM and other committers) and seek their help, ask on the dev@ list. I saw no email from this contributor on dev@, I know they've pinged me and few others - however, I wasn't free to focus on 4.16 amidst my work and personal life and I may not be in a position to always work when anybody demands my time. In fact all pings such as those made on this PR to my github handle end up in my inbox (have 500+ emails which I'm not able to read) that I don't actively check, however I do check dev@ more diligently. Now that @nvazquez is the RM for 4.16 it gives me even lesser incentive to be involved in all PRs/issues.

I don't think there is scheme to ignore "new" contributors @GabrielBrascher, however it's just practically not possible to ensure everybody gets attention, I would say all the RMs at least do try in a structured way. However, I would say old contributors do have the benefit that they know how to build support, how to engage with the community and get active contributors/committer/PMC attention to get their PRs reviewed/tested/merged.

yeah, I missed the smoke tests @rhtyd. I did test it manually though. I'll keep an eye on the regression part on health check PR.

Hi guys,

I was not against this PR or its author @RodrigoDLopez, as it was proposed to close this PR in favour of another one I wanted his opinion about that.

@harikrishna-patnala please make sure to check smoke tests results before merging so we can maintain a healthy main branch and detect any issues earlier

To give some extra context, my comment was related to one of the main concerns in Apache's projects: work towards community and encourage new contributors to step in.

@rhtyd to answer your points, let's break into parts:

I've not looked into the code or tested this, but this PR removes checks and can potentially cause regressions, and introduces a change in behavior.

This PR is actually reverting a regression issue that broke an existing feature in the API (see the original implementation that this PR was getting back #2636). For more details, please check out PR #4257 description and discussion during the review process.

New contributors should learn to build support, learn how to improve their code and communicate in a way to get their PRs merged. I would ask them to start by identify who the active contributors in the project are (usually the RM and other committers) and seek their help, ask on the dev@ list.

First of all, committers and (especially) PMCs, should work together to keep the project traction and follow the ASF way. That includes, in my opinion, extra attention with new contributors so they develop into the community and can become new committers in the future. I think that we should step up and give our best to guide new contributors.

With that said, @RodrigoDLopez did "know how to build support". He did a perfect job of creating a clear description, test scenarios, researching the git history and other PRs, reaching out to committers and contributors, and keeping his PR updated for over a full year. I don't see why you bring this point as if he did not "learned to build support". This clearly is not the case here.

I wasn't free to focus on 4.16 amidst my work and personal life and I may not be in a position to always work when anybody demands my time.

We have a clear definition of how to proceed with PRs. 2 LGTMs and a CI test. This PR had 3 LGTMs and was just missing the CI. Therefore the point was not related specifically to contributor A or B coming here to review. It was regarding not putting a PR aside where it already matches all the needed criteria.

Very simple reason - discussion/support building didn't happen on the dev list, nor smoketest were requested to run or run/shared by the author using Trillian or mbx.

We have a clear definition of how to proceed with PRs. 2 LGTMs and a CI test. This PR had 3 LGTMs and was just missing the CI. Therefore the point was not related specifically to contributor A or B coming here to review. It was regarding not putting a PR aside where it already matches all the needed criteria.

We do also require a 3rd person test of the functionality, as I understand it. It makes sense to be practical about that and I'm not sure if it is relevant in this discussion. I'm just adding it here to be complete.
About this PR, let's move on and deal with any problems as we go.
About the discussion of good practice that seems to evolve here, that should happen on dev@cloudstack.apache.org .