-
Notifications
You must be signed in to change notification settings - Fork 447
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
Make it possible to add annotations to the keys secret #3484
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
/azp run coretools.public |
Azure Pipelines successfully started running 1 pipeline(s). |
lgtm but I don't want to merge this in without tests; you should be able to write unit tests on a mac (it's my primary device as well). If you're not longer able to contribute to this PR, I can speak to the team about getting this reassigned! |
I believe it is outdated. It was written 7 years ago. My team just got ownership of this repo so we can work on updating this outdated information, but this might take some time given other priorities. You should be able to run tests like any normal c# project |
@liliankasem I just tried getting the tests to run, or even
<Target Name="CopyInProc8" AfterTargets="Build" Condition="'$(TargetFramework)'=='net6.0'">
<Exec Command="xcopy /Y /I /E "$(MSBuildThisFileDirectory)..\..\src\Azure.Functions.Cli\bin\$(Configuration)\$(TargetFramework)\in-proc8\*" "$(OutDir)in-proc8\"" />
</Target> which for obvious? reasons is not really working on OSX making the tests work at all seems a bit out of scope for this PR IMO. |
if (eventInfo.Source.Type == typeof(string) && double.TryParse(eventInfo.Source.Value.ToString(), out _)) | ||
{ | ||
eventInfo.Style = ScalarStyle.DoubleQuoted; | ||
} else if (eventInfo.Source.Type == typeof(string) && bool.TryParse(eventInfo.Source.Value.ToString(), out _)) | ||
{ | ||
eventInfo.Style = ScalarStyle.DoubleQuoted; | ||
} | ||
|
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.
if (eventInfo.Source.Type == typeof(string) && double.TryParse(eventInfo.Source.Value.ToString(), out _)) | |
{ | |
eventInfo.Style = ScalarStyle.DoubleQuoted; | |
} else if (eventInfo.Source.Type == typeof(string) && bool.TryParse(eventInfo.Source.Value.ToString(), out _)) | |
{ | |
eventInfo.Style = ScalarStyle.DoubleQuoted; | |
} | |
if (eventInfo.Source.Type == typeof(string) && | |
(double.TryParse(eventInfo.Source.Value.ToString(), out _) || | |
bool.TryParse(eventInfo.Source.Value.ToString(), out _))) | |
{ | |
eventInfo.Style = ScalarStyle.DoubleQuoted; | |
} |
Sorry for the delay here, I missed this. this code snippet no longer exists so that shouldn't be an issue. But tests on osx are still a pain :/ Given it's such a minor change and we are in the middle of refactoring the test project, I think it's okay to merge. |
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.
minor nit on if statement format
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -93,6 +95,7 @@ public override ICommandLineParserResult ParseArgs(string[] args) | |||
SetFlag<string>("config-file", "if --write-configs is true, write configs to this file (default: 'functions.yaml')", f => ConfigFile = f); | |||
SetFlag<string>("hash-files", "Files to hash to determine the image version", f => HashFilesPattern = f); | |||
SetFlag<bool>("image-build", "If false, skip the docker build", f => BuildImage = f); | |||
SetFlag<string>("keys-secret-annotations", "The annotations to add to the keys secret e.g. key1=val1,key2=val2", a => KeysSecretAnnotations = a.Split(',').Select(s => s.Split('=')).ToDictionary(k => k[0], v => v[1])); |
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.
nit:
SetFlag<string>("keys-secret-annotations", "The annotations to add to the keys secret e.g. key1=val1,key2=val2", a => KeysSecretAnnotations = a.Split(',').Select(s => s.Split('=')).ToDictionary(k => k[0], v => v[1])); | |
SetFlag<string>("key-secret-annotations", "The annotations to add to the keys secret e.g. key1=val1,key2=val2", a => KeySecretAnnotations = a.Split(',').Select(s => s.Split('=')).ToDictionary(k => k[0], v => v[1])); |
Add command line option
--keys-secret-annotations
for specifying the annotations to be added to the keys secret when doingfunc kubernetes deploy
Tested manually using
--keys-secret-annotations reflector.v1.k8s.emberstack.com/reflection-allowed=true,reflector.v1.k8s.emberstack.com/reflection-auto-enabled=true,reflector.v1.k8s.emberstack.com/reflection-auto-namespaces=default
which renders the relevant bit of the keys secret like follows
Issue describing the changes in this PR
resolves #3483
Pull request checklist
I have not ran nor added tests as I have a mac.