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

Stream handling, param block, PowerShellManager #11

Merged
merged 17 commits into from
Aug 29, 2018

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Aug 28, 2018

This PR accomplishes 3 things (that could have been 3 PRs... but that's on me)

  • Switches from using as custom host for logging to using Stream handlers on the DataAdding event
  • Switches to passing in the parameters via a param block per our discussion with @joeyaiello
  • The PowerShellManager object which is responsible for managing the PowerShell instance we use to execute functions. It contains:
    • setting up the logging
    • executing powershell script in the runspace
    • clean/reseting the runspace
    • sets the stage for concurrency if/when we go down that road

I know there's a lack of documentation and associated tests but I plan on adding those in a separate PR to address #6

}
}

public void ErrorDataAdding(object data, DataAddingEventArgs e)
Copy link
Contributor

Choose a reason for hiding this comment

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

The first parameter of an event handler should be object sender.


// Add HttpResponseContext namespace so users can reference
// HttpResponseContext without needing to specify the full namespace
ExecuteScriptAndClearCommands($"using namespace {typeof(HttpResponseContext).Namespace}");
Copy link
Contributor

Choose a reason for hiding this comment

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

When initing the worker, we need to run Set-ExecutionPolicy when running on windows, because by default the policy is restricted, so no script can be executed.
We will use a Linux container, but still better to future proof.

Copy link
Contributor

Choose a reason for hiding this comment

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

And no need to run this using namespace more than once for a runspace.

_logger = logger;
}

public static PowerShellManager Create(PowerShell pwsh, RpcLogger logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me you don't really need pwsh to be passed in.

pwsh.Streams.Verbose.DataAdding += streamHandler.VerboseDataAdding;
pwsh.Streams.Warning.DataAdding += streamHandler.WarningDataAdding;

manager.ResetRunspace();
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess no need to call reset when creating the PowerShellManager.

_logger.LogTrace(logMessage);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new line to the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Write-Warning "Warning $name"
# you can write to the same streams as you would in a normal PowerShell script
Write-Verbose "Verbose $name" -Verbose
Write-Warning "Warning $i"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is $i coming from?

Copy link
Member Author

Choose a reason for hiding this comment

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

d'oh!

Copy link
Contributor

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM for now. Will need to rewrite some of the code after moving to the module approach.

@TylerLeonhardt
Copy link
Member Author

Thanks @daxian-dbw!

@TylerLeonhardt TylerLeonhardt merged commit 58924c8 into dev Aug 29, 2018
@TylerLeonhardt TylerLeonhardt deleted the params-and-powershellmanager branch August 29, 2018 23:01
davidmrdavid added a commit that referenced this pull request Apr 7, 2022
# This is the 1st commit message:

separate DF SDK classes from DF worker classes

# This is the commit message #2:

fix typo

# This is the commit message #3:

DurableSDK now compiles by itself

# This is the commit message #4:

Allow ExternalSDK to handle orchestration

# This is the commit message #5:

document next steps

# This is the commit message #6:

allow external SDK to set the user-code's input. Still need to refactor this logic for the worker to continue working with old SDK

# This is the commit message #7:

add import module

# This is the commit message #8:

supress traces

# This is the commit message #9:

avoid nullptr

# This is the commit message #10:

pass tests

# This is the commit message #11:

fix E2E tests

# This is the commit message #12:

develop E2E tests

# This is the commit message #13:

Enabled external durable client (#765)

Co-authored-by: Michael Peng <[email protected]>
# This is the commit message #14:

bindings work

# This is the commit message #15:

conditional binding intialization

# This is the commit message #16:

conditional import

# This is the commit message #17:

Added exception handling logic

# This is the commit message #18:

Revert durableController name to durableFunctionsUtils

# This is the commit message #19:

Ensure unit tests are functioning properly

# This is the commit message #20:

Corrected unit test names

# This is the commit message #21:

Turned repeated variables in unit tests into static members

# This is the commit message #22:

Fixed issue with building the worker

# This is the commit message #23:

Fix E2E test

# This is the commit message #24:

Fixed unit test setup

# This is the commit message #25:

Fixed another unit test setup

# This is the commit message #26:

Remove string representation of booleans

# This is the commit message #27:

patch e2e test
# This is the commit message #28:

remove typo in toString
# This is the commit message #29:

Update PowerShell language worker pipelines (#750)

* Install .Net to a global location

* Remove .Net installation tasks

* Update install .Net 6 task

* Update Windows image to use windows-latest
# This is the commit message #30:

Make throughput warning message visible for tooling diagnosis (#757)


# This is the commit message #31:

Update grpc.tools to version 2.43.0

# This is the commit message #32:

Update Google.Protobuf.Tools to version 3.19.4

# This is the commit message #33:

Revert "Update Google.Protobuf.Tools to version 3.19.4"

This reverts commit bcbd022.

# This is the commit message #34:

Revert "Update grpc.tools to version 2.43.0"

This reverts commit ccb323a.

# This is the commit message #35:

Update Google.Protobuf to 3.19.4 and grpc.tools to  2.43.0 (#762)

* Update grpc.tools to version 2.43.0

* Update Google.Protobuf.Tools to version 3.19.4
# This is the commit message #36:

Switch from Grpc.Core to Grpc.Net.Client (#758)

* Upgraded protobuf versions and removed Grpc.Core dependency

* Updated channel and option types used

* Change channel credentials

* Added http prefix to url

* Add valid URL check and explicitly include credentials
# This is the commit message #37:

Update pipeline logic to generate the SBOM for release builds (#767)
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.

2 participants