-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
PowerShell Client Module #1990
PowerShell Client Module #1990
Conversation
Nice! |
<Reference Include="System.Management.Automation, Version=3.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> | ||
<HintPath>..\packages\System.Management.Automation.dll.10.0.10586.0\lib\net40\System.Management.Automation.dll</HintPath> | ||
<Private>True</Private> | ||
</Reference> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In master
we transitioned to using project.json
for the nuget packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember you said that the nuget consumers like samples for example would still be on the packages.config. Ok will change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this is not a nuget consumer, or did I get it wrong? I thought you were adding this to the main Orleans.sln
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you are right. Will change that.
This looks good to me. |
LGTM. @galvesribeiro please squash before we can merge. @jdom, I don't see any issues with merging this, only benefits. |
@shayhatsor done. Ah! Remember that while merging PRs GH can squash for you :) |
@@ -10,7 +10,8 @@ | |||
"WindowsAzure.Storage": "7.0.0", | |||
"xunit": "2.1.0", | |||
"xunit.runner.visualstudio": "2.1.0", | |||
"Xunit.SkippableFact": "1.2.14" | |||
"Xunit.SkippableFact": "1.2.14", | |||
"System.Management.Automation.dll": "10.0.10586" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this one doesn't require anything from Tester, should we start creating a new test project for the Powershell DLL? I think that's going to be how we do things in the future, and it would help us in the short term since this is not portable to .NET Core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It requires a cluster to run. Should we duplicated that test cluster stuff in another project? If yes I can move it right away and update the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we can move those utilities to another project, or if you don't want to take this work in this PR, you can do the same thing as TesterInternal, and have this new test project reference Tester in order to use BaseTestClusterFixture
.
We can refactor BaseTestClusterFixture
and some other extra stuff into their own project later. I believe that would be more appropriate instead of conflating that move in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the change was lost when rebasing? I thought you were creating a new test project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for now. Will keep it there. We need revisit all the tests and make a test kit otherwise we will still keeping the tests duplicated but this time, spread across multiple projects.
I think we should design a new testkit I'm just not sure this is the right time since we have tons of other things to do for the .net core move...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not suffer from the same as the other provider tests and does not require any test kit. It would be trivial to just move to a new test project in the way I suggested.
We'll then have to define the steps required to publish to the gallery. I assume these can be automated? |
|
||
namespace Tester | ||
{ | ||
public class PowershellHostFixture : DefaultClusterFixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably don't want to inherit from DefaultTestCluster fixture, and instead use it's base class and start the Silo cluster but not necessarily the grain client (I believe there's an overload to TestCluster's constructor where you just use the ClusterConfiguration but just pass a null ClientConfiguration). You can take the ClusterConfiguration object from the TestClusterOptions.ClusterConfiguration property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this a regression? I thought you had an iteration where you were no longer inheriting from Default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outdated @jdom. The current code in the PR already has the change you suggested and inherit from BaseTestClusterFixture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the process is the same as publish a nuget. You just call a command line that pass the build output path and the publish key just as nuget. Lets talk offline and I can make another PR for it once this is merged. |
} | ||
|
||
[Fact, TestCategory("BVT"), TestCategory("Tooling")] | ||
public void GetGrainTest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetGrainTest [](start = 20, length = 12)
It could be nice to test all possible values for key type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjaminpetit you mean test each type of key supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you could try to get a IGuidGrain
, IStringGrain
, ect. Just to cover the switch statement in GetGrain.ProcessRecord
@jdom and @benjaminpetit added the suggested changes. |
@@ -0,0 +1,24 @@ | |||
@{ | |||
RootModule = 'OrleansPSUtils.dll' | |||
ModuleVersion = '1.2.3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to have this populated by the version of the OrleansPSUtils (effectively the same as the rest of Orleans)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed that earlier.
It could be nice to have this value synced with what is inside Build\GlobalAssemblyInfo.cs
. I believe it could be done by writing a custom target in the csproj.
Same for the version in ReleaseNotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least update to 1.3.0, I think it will be enough for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do this:
<UsingTask TaskName="UpdateVersion" TaskFactory="CodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.v4.0.dll">
<ParameterGroup>
<InputFilename ParameterType="System.String" Required="true" />
<OutputFilename ParameterType="System.String" Required="true" />
</ParameterGroup>
<Task>
<Reference Include="System.Core" />
<Using Namespace="System" />
<Using Namespace="System.IO" />
<Using Namespace="System.Text.RegularExpressions" />
<Code Type="Fragment" Language="cs"><![CDATA[
var version = File.ReadAllText(InputFilename);
var inputText = File.ReadAllTest(OutputFilename);
File.WriteAllText(
OutputFilename,
inputText.Replace("$version$", version)
);
]]></Code>
</Task>
</UsingTask>
and in the end of the .csproj this:
<Target Name="AfterBuild">
<ReplaceFileText InputFilename="$(SolutionDir)Build\Version.txt" OutputFilename="$(OutputDir)Orleans.psd1" />
</Target>
I'm not good at MSBuild so if you can put something together for it I can quickly add it here.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works:
<UsingTask TaskName="UpdateVersion" TaskFactory="CodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.v4.0.dll">
<ParameterGroup>
<InputFilename ParameterType="System.String" Required="true" />
<OutputFilename ParameterType="System.String" Required="true" />
</ParameterGroup>
<Task>
<Reference Include="System.Core" />
<Using Namespace="System" />
<Using Namespace="System.IO" />
<Code Type="Fragment" Language="cs">
<![CDATA[
var version = File.ReadAllText(InputFilename)
.Replace("\r\n", "")
.Replace("\n", "")
.Replace("\r", "");
var inputText = File.ReadAllText(OutputFilename);
File.WriteAllText(
OutputFilename,
inputText.Replace("$version$", version)
);
]]></Code>
</Task>
</UsingTask>
and
<Target Name="AfterBuild">
<UpdateVersion InputFilename="$(SolutionDir)Build\Version.txt" OutputFilename="$(OutputPath)Orleans.psd1" />
</Target>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! My mistake was the AfterBuild that was in the wrong place and ofc the endline replaces you add. Now it worked like a charm :)
|
Build failure looks like the MSBuild task is breaking it on the CI mahine... @benjaminpetit / @jdom any idea? |
It seems that when you build with MSBuild the But we have to find a way to copy all output from this project in a dedicated subfolder, right? Because right now with |
Yes, We need have a folder that will end up on the package. So when we call the pack command, it will build the package with this folder so later on, while installing, it will be |
Adding this in the csproj:
seems to do exactly what we want! |
@benjaminpetit any particular position? |
Just before the first |
@benjaminpetit done! Thanks for the help! |
LGTM |
Thanks @galvesribeiro !! |
Thanks @galvesribeiro, great contribution. LGTM, I'll merge right after you move this to it's own test project |
Doh, didn't realized @shayhatsor had merged already. In the future, @shayhatsor, please give time for other reviewers to validate their comments after the submitter amends the PR. In this case there were still unaddressed comments. Nothing major in this case, but now it puts the pressure on us to fix it right away. |
@jdom it was already merged. Also, related to the tests, until we have fully extracted the testkit, I see no point on have separated projects just to duplicate test code. |
@galvesribeiro, in the future please when you address comments with new changes, please do not rebase every time. In some cases it caused regressions, but regardless of that, it's hard to know what are the new changes since the last time I reviewed it, and had to review all over again. We can use the squash and merge button anyway. It would only require rebasing if there's conflicts that do not allow the merge |
@jdom, I was under the impression that @galvesribeiro had already addressed all issues. I apologize for jumping the gun. |
not sure I understand why you would need a test kit in this case. It's not a provider, you don't need to reuse any of the tests (which is what the test kit would address) |
No problem, btw, I'm just mentioning this for future practice. I'll put out a PR on Monday to address the remaining comment, otherwise it provides more friction to the .net core migration. |
@jdom Maybe put something to contributor guidelines? It might be useful to have something like:
Perhaps with even more detailed instructions (and having Then also how one should open PRs and how in general it is expected it to work. It might make it go smoother. There are probably hesistant people that try to search "how to do it correctly" and we could give instructions. But also tell that it's OK if things go a bit wrong in the process, everything can be fixed. |
Hmm, unfortunately this merge broke most of our builds (functionals, load tests, reliability), since some of them still use VSO's way of building that puts everything in a single folder, and I guess the OutDir variable is not being set.
This is why we were going to run this PR by our build system before merging... Unfortunately it is very common that when we add build tasks, some issues arise. We are trying to get rid of this way of building in some of our tests, but we are not there yet. Could someone please lend a hand to fix this? Otherwise we are blocked on running tests for other PRs and we'll either have to drop everything and do this emergency fix or revert this for now. I'd prefer not to revert if someone is willing to help asap. |
Without understand and have access to VSO I can't say what variables VSO is using that is making that don't work. BTW, lets just be clear. The merge doesn't break anything by itself. The common consensus in the past was that we had ALL the build processes working seamless between both Jenkins and VSO and we spent weeks on that migration. If there is something old there, we would never guess since we don't have access to it :( |
Let me clarify @galvesribeiro: you did nothing wrong. Nothing at all. |
If this merge is causing any problem with internal builds, I see no harm in reverting for now. |
I'm troubleshooting now to avoid the revert, I think I'm close, just running the tests. Yeah, both new projects and new build dependencies (whether it's build scripts, msbuild tasks, test scripts). Also external environmental dependencies. |
Nevermind, I wasn't close. @benjaminpetit is taking a look at it now |
I can't actively help on that since I cant simulate the problem here... If there is a way for me to do that let me know |
@galvesribeiro Do you know what's required to publish to the gallery? Signed build? We have one. Do we also need to sign any powershell script? |
@jdom sorry for the delay. Just create an account on powershellgallery.com which is pretty similar to the nuget.org one and grab an APIKey. Invoke the Publish-Module cmdlet and fill all the parameters and you should all be set. Regarding the signature, I would suggest you to sign the assembly. This project output a .Net Assembly just like any other class library, so the sign process should be the same. There is no real .ps1 script. Let me know if you have any problems. |
This initial implementation is part of a series of improvements on tooling.
Many people were asking on Gitter for a way to interact with grains from a console.
This initial push enables user to start and stop
GrainClient
and call any grain from Powershell.Currently GrainObservers, Streams, interceptors are not implemented. This first step allow only direct calls to grain methods as in any other client that uses
GrainClient
does.A page will be added to docs site on a separated PR explaining how to use it and more work will come to support other client scenarios.