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

Revert "Fixes #1372" #1377

Merged

Conversation

IsakNaslundBh
Copy link
Contributor

Issues addressed by this PR

Closes #1376

This reverts commit ba775cc.

Reverting previously merged commit due to problems in GH

Test files

Changelog

Additional comments

This reverts commit ba775cc.
@IsakNaslundBh IsakNaslundBh added severity:critical No workaround exists. Essential to continue type:bug Error or unexpected behaviour labels Dec 5, 2019
@IsakNaslundBh IsakNaslundBh added this to the BHoM 3.0 β MVP milestone Dec 5, 2019
@IsakNaslundBh IsakNaslundBh self-assigned this Dec 5, 2019
@IsakNaslundBh
Copy link
Contributor Author

/azp run BHoM_Engine.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@epignatelli epignatelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the plan the resolve the issue this fix originally handled? I can't see that here, while I accept this fix has caused some unintended consequences that our current CI can't catch, we shouldn't just revert a fix without a transparent plan for resolving the original issue.

@IsakNaslundBh
Copy link
Contributor Author

IsakNaslundBh commented Dec 5, 2019

What is the plan the resolve the issue this fix originally handled? I can't see that here, while I accept this fix has caused some unintended consequences that our current CI can't catch, we shouldn't just revert a fix without a transparent plan for resolving the original issue.

The plan is to re-raise the previous PR again and make sure it works as expected as well as making sure GH works.
Right now no engine component is loaded up in GH when building from master as well as using last night alpha installer.

The suggested action is:

  1. Revert this as much more critical to have Engine loading in GH at all
  2. Re-open the previously merged issue and make a new PR ensuring this does not happen

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am happy with response provided, if we can get that issue reopened ASAP so it's not lost that's be great.

@IsakNaslundBh IsakNaslundBh merged commit 0d795e0 into master Dec 5, 2019
@IsakNaslundBh IsakNaslundBh deleted the Reflection_Engine-#1376-EngineDoesNotLoadInGH branch December 5, 2019 11:46
@alelom
Copy link
Member

alelom commented Dec 5, 2019

Am happy with response provided, if we can get that issue reopened ASAP so it's not lost that's be great.

Issue reopened by @IsakNaslundBh
new PR targeting that #1378

@al-fisher
Copy link
Member

Thanks @IsakNaslundBh, @epignatelli, all, for working to swiftly to resolve this as it emerged. Well done!
Good to capture some planned improvements to testing procedure (there are always improvements to be made! Even as we continue to make massive advances!) to reduce further likelihood of this!
Will raise an issue for discussion and any additions for next quarter!
Great stuff 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:critical No workaround exists. Essential to continue type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reflection_Engine: Engine does not load in GH
5 participants