Skip to content

fix load_parameter_dict_from_yaml test failure on windows#956

Closed
ihasdapie wants to merge 2 commits intomasterfrom
brianc/parameter_client_fix
Closed

fix load_parameter_dict_from_yaml test failure on windows#956
ihasdapie wants to merge 2 commits intomasterfrom
brianc/parameter_client_fix

Conversation

@ihasdapie
Copy link
Copy Markdown
Member

Resolves windows test failure for load_parameter_dict_from_yaml_file caused by windows handling of tempfiles. (See: #945 (comment))

I've also bundled in a small fix in this PR to change the default depth of list_parameters to the full depth instead of 1 which to me is a saner default.

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
Copy link
Copy Markdown
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Collaborator

Basically I believe that each PR should have complete fix w/o build break for maintenance. this could be problem when we do cherry-pick.

@clalancette @Blast545 what do you think?

@ihasdapie
Copy link
Copy Markdown
Member Author

I think that would be good too, especially since I forgot to hit squash merge on the original PR 😅. But reverting would involve rewriting history which wouldn't be good

@clalancette
Copy link
Copy Markdown
Contributor

I think that would be good too, especially since I forgot to hit squash merge on the original PR sweat_smile. But reverting would involve rewriting history which wouldn't be good

Not quite. We can do a git revert, which doesn't rewrite history but makes a new commit that is the opposite of the original commit. Then we could do a new PR, including the original PR and this fix. That maintains history, and also keeps what @fujitatomoya is asking for.

I don't feel strongly about it either way, but I would strongly prefer that by end of day we either a) get the revert in, or b) get this fix in (I notice that it is failing all CI).

@ihasdapie
Copy link
Copy Markdown
Member Author

ihasdapie commented Jun 15, 2022

I don't have any preference as well, @fujitatomoya maybe just do what you think is best?

I passed in the wrong argument to CI, try these ones instead:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ihasdapie
Copy link
Copy Markdown
Member Author

@fujitatomoya I've created a revert PR at #958 and I will submit a new PR with the new feature & all the fixes included

ihasdapie added a commit that referenced this pull request Jun 15, 2022
@ihasdapie ihasdapie closed this Jun 15, 2022
ihasdapie added a commit that referenced this pull request Jun 17, 2022
See discussion @ #956

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie ihasdapie deleted the brianc/parameter_client_fix branch June 20, 2022 23:03
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.

3 participants