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

Fix http error parser in the client #36

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Conversation

amaltaro
Copy link
Contributor

@amaltaro amaltaro commented Nov 3, 2021

Fixes dmwm/WMCore#10887

Fix also provided in the old DBS repository: dmwm/DBS#660

@vkuznet
Copy link
Contributor

vkuznet commented Nov 3, 2021

@amaltaro can you provide another PR without any formatting changes? If this PR will be merged I need to redo entire set of changes in DBSClient4go branch since you change all the formatting and this will force me to redo changes once again. I think we follow the rule in WMCore that formatting should be done after the fix is applied, and certainly this should be the case here too since it will break the merge with other branch.

@amaltaro
Copy link
Contributor Author

amaltaro commented Nov 3, 2021

The rule in WMCore is to simply separate them in different commits, not in different PRs.

Valentin, that's fine with me. I see the following alternatives, and I wait for your decision on how you prefer to proceed:
a) I remove the second commit from this PR (keeping only the actual change/fix)
b) I apply the same formatting changes to the DBSClient4Go branch
c) (your request) I create another PR against main branch only with the first commit from this PR.

Please let me know your preference. Thanks

@vkuznet
Copy link
Contributor

vkuznet commented Nov 3, 2021

I suggest we will do the following:

  • please remove second comment, and leave only actual fix
  • either create the same fix changes against DBSClient4Go branch or I'll port them (based on your commit to DBSClient4Go branch)
  • then, we will merge DBSClient4Go branch
  • then, you will create PR with formatting changes

@amaltaro
Copy link
Contributor Author

amaltaro commented Nov 3, 2021

Valentin, the actual fix has been provided already to the DBSClient4Go branch. Check it out here:
#37

I'm now going to remove this 2nd commit from this PR, and put it in a different PR.

@amaltaro
Copy link
Contributor Author

amaltaro commented Nov 3, 2021

And here is the code reformat only PR: #39

@yuyiguo
Copy link
Member

yuyiguo commented Nov 3, 2021

replaced by two PRs:#37 and #39.

@yuyiguo yuyiguo closed this Nov 3, 2021
@amaltaro
Copy link
Contributor Author

amaltaro commented Nov 3, 2021

@yuyiguo actually this one can be merged today, since it has a bug fix to parse failed http calls (and feel free to provide a new tag/pypi package, please).

PR #39 is the one that we should not merge until Valentin's changes can be merged into the main branch

@yuyiguo
Copy link
Member

yuyiguo commented Nov 3, 2021

@amaltaro
this one is the same as #37 and #37 is merged. If they are different, please let me know.

@amaltaro
Copy link
Contributor Author

amaltaro commented Nov 3, 2021

They were made against different branches.

@yuyiguo
Copy link
Member

yuyiguo commented Nov 3, 2021

oh, my fault.
Do you need a new release now from the main branch? @amaltaro

@vkuznet
How far are you from meger DBSClient4Go into main?

@yuyiguo yuyiguo reopened this Nov 3, 2021
@vkuznet
Copy link
Contributor

vkuznet commented Nov 3, 2021

@yuyiguo I plan to make another round of DBSClient4go branch changes against cmsweb-testbed tomorrow and then I can merge it (today I'm off for the rest of the day).

@yuyiguo
Copy link
Member

yuyiguo commented Nov 3, 2021

@vkuznet Thanks for the update. I just wondered if we should build a new release against main now or wait for the merging.
@amaltaro, let me know if a new release is needed before the merging.

@vkuznet
Copy link
Contributor

vkuznet commented Nov 3, 2021

@yuyiguo I suggest to build new release before the merge, this will give time for everyone and mark stable release before the changes required for Go server.

@amaltaro
Copy link
Contributor Author

amaltaro commented Nov 3, 2021

@yuyiguo I suggest to build new release before the merge, this will give time for everyone and mark stable release before the changes required for Go server.

Agreed! It seems to be 3.17.6 + this bugfix. That should give us a stable release based on this new repository separation.

@yuyiguo
Copy link
Member

yuyiguo commented Nov 4, 2021

@amaltaro @vkuznet
Good idea, I will build a new release and let everyone know.

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.

Blocks failing to be inserted into DBS - submit4
3 participants