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 load_parameter_dict_from_yaml test failure on windows #956

Closed
wants to merge 2 commits into from

Conversation

ihasdapie
Copy link
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.

@ihasdapie
Copy link
Member Author

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

@fujitatomoya
Copy link
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
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
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
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
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 <[email protected]>
@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