You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/08/10 20:55:16 UTC
[GitHub] [cloudstack] RodrigoDLopez opened a new pull request #4257: remove the unnecessary check for tags when migrating volumes
RodrigoDLopez opened a new pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257
## Description
With this commit [Fix limitation on tag matching in 'migrateVolume' with disk offering](https://github.com/apache/cloudstack/commit/756a7e89cbcec7a1aa98b3f470cbdf8181af75ad) the author changed the volume migrate API to allow operators to override the volume placement. Moreover, the tag validation would only happen if the operator changes the disk offering. However, further, but with this commit [Vmware offline migration](https://github.com/apache/cloudstack/commit/b363fd49f70ac2092ebe6226a72a3d911dc99e1f), cloudstack started to validate the tags again when the operator executes the volume migrate command without. This broke the override mechanism that has been previously introduced into ACS.
If the operator wants to migrate volumes, without changing the disk offering, we don't need to check if the tag match with the storage pool tag. As discussed merged in the past, the operator should be able to override the disk placement (without changing the disk offering). So this PR removes this unnecessary check for tags preventing `Unexpected exception while executing MigrateVolumeCmd`.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviors, steps to reproduce. -->
**Steps to reproduce**
To reproduce this error is necessary:
* DO: a disk offering with tag 'A';
* SP1: a Storage pool with tag 'A';
* SP2: a Storage pool with tag 'B';
* Create a volume using 'DO' disk offering, this will be allocated at SP1;
* Try to migrate this volume to SP2
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## How Has This Been Tested?
To run tests, I used `cloudmonkey` to migrate volumes
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
**Without this PR**
We can not migrate de volume, because of that unnecessary verification
```
() 🐱 > migrate volume livemigrate=true volumeid=a0f4e7f8-56d2-408b-9ef4-34461ff59b93 storageid=5154b842-5d4f-3589-af96-b3e0f6b82e55
{
"accountid": "d9feed3f-88a1-11ea-85e0-525400ab8632",
"cmd": "org.apache.cloudstack.api.command.admin.volume.MigrateVolumeCmdByAdmin",
"completed": "2020-08-07T11:09:06-0400",
"created": "2020-08-07T11:09:05-0400",
"jobid": "6d6b3880-54a1-4345-a178-323182c67b12",
"jobprocstatus": 0,
"jobresult": {
"errorcode": 530,
"errortext": "Migration target pool [null, tags:ssd2] has no matching tags for volume [ROOT-90, uuid:a0f4e7f8-56d2-408b-9ef4-34461ff59b93, tags:ssd]"
},
"jobresultcode": 530,
"jobresulttype": "object",
"jobstatus": 2,
"userid": "d9ffc914-88a1-11ea-85e0-525400ab8632"
}
```
**With this changes**
All good, as expected.
```
() 🐱 > migrate volume livemigrate=true volumeid=a0f4e7f8-56d2-408b-9ef4-34461ff59b93 storageid=5154b842-5d4f-3589-af96-b3e0f6b82e55
{
"volume": {
"account": "admin",
"clusterid": "a593b11e-d89e-41a3-9626-88250d7ca927",
"clustername": "Cluster",
"created": "2020-08-07T11:15:18-0400",
"destroyed": false,
"deviceid": 0,
"displayvolume": true,
"domain": "ROOT",
"domainid": "7243f453-889e-11ea-85e0-525400ab8632",
"hypervisor": "KVM",
"id": "a0f4e7f8-56d2-408b-9ef4-34461ff59b93",
"isextractable": false,
"name": "ROOT-90",
"path": "53f80541-0819-4628-9318-3c570b5e9656",
"podid": "3a0f99ab-c0f4-4317-bdfa-f1c0396acb76",
"podname": "Pod",
"provisioningtype": "thin",
"quiescevm": false,
"serviceofferingdisplaytext": "tiny with tag",
"serviceofferingid": "18b614bf-9104-4bc0-98b0-24cecdfb2a41",
"serviceofferingname": "tiny with tag",
"size": 214749184,
"state": "Ready",
"storage": "primary2",
"storageid": "5154b842-5d4f-3589-af96-b3e0f6b82e55",
"storagetype": "shared",
"tags": [],
"templatedisplaytext": "tiny",
"templateid": "ee7531b0-9d6f-4021-a17f-07ceb183b6d6",
"templatename": "tiny",
"type": "ROOT",
"virtualmachineid": "9ae4614f-2917-4eb1-a0ff-752db65cf624",
"vmdisplayname": "VM-9ae4614f-2917-4eb1-a0ff-752db65cf624",
"vmname": "VM-9ae4614f-2917-4eb1-a0ff-752db65cf624",
"vmstate": "Stopped",
"zoneid": "9ee874c2-0425-4d65-aa09-6ab7a9d1040e",
"zonename": "Zone"
}
}
```
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-696452264
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-696452264
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-762742083
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-883009246
@RodrigoDLopez @harikrishna-patnala please advise if this PR should be closed in favour of #5008
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-888876472
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-815255676
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-889227988
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-883009246
@RodrigoDLopez @harikrishna-patnala please advise if this PR should be closed in favour of #5008
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] harikrishna-patnala merged pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala merged pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-881223699
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd edited a comment on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rhtyd edited a comment on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-817104539
I did a highlevel check I hesitate with removing these checks, @sureshanaparti - do you want to add some ideas we recently discussed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-888602105
I don't see why this PR should be closed in favour of https://github.com/apache/cloudstack/pull/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:
- https://github.com/apache/cloudstack/pull/4283
- https://github.com/apache/cloudstack/pull/4282
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez edited a comment on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez edited a comment on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-859593681
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-673476420
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez closed pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-874073693
ping @rhtyd @GabrielBrascher @sureshanaparti @DaanHoogland @weizhouapache
Any update on this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-888957821
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-889276668
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-694866413
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-673495264
Packaging result: ✔centos7 ✔debian. JID-1743
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-859593681
> 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 thos ideas with me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-673476990
@RodrigoDLopez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-883009246
@RodrigoDLopez @harikrishna-patnala please advise if this PR should be closed in favour of #5008
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-723148403
@PaulAngus @rhtyd can you consider this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-762386479
@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...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-889293050
> 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 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-817104539
I did a highlevel check I hesistate with removing these checks, @sureshanaparti - do you want to add some ideas we recently discussed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-696452264
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-888573963
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez closed pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez closed pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-694866802
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-879604507
I'm working on this PR https://github.com/apache/cloudstack/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-874073693
ping @rhtyd @GabrielBrascher @sureshanaparti @DaanHoogland @weizhouapache
Any update on this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-889230975
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-888790145
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-878832139
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-694884641
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2040
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-887185414
Ping @RodrigoDLopez @harikrishna-patnala
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-754018183
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4257: remove the unnecessary check for tags when migrating volumes
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4257:
URL: https://github.com/apache/cloudstack/pull/4257#issuecomment-781931558
Hi @RodrigoDLopez sorry busy with the dot releases, I'll revisit this soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org