feat: add image and manifest for windows npm #1098
Conversation
npm/npm.go
Outdated
| podInformer: informerFactory.Core().V1().Pods(), | ||
| nsInformer: informerFactory.Core().V1().Namespaces(), | ||
| npInformer: informerFactory.Networking().V1().NetworkPolicies(), | ||
| ipsMgr: ipsm.NewIpsetManager(exec), |
There was a problem hiding this comment.
Can we move this out from general and put in V1 controller section ?
| } | ||
|
|
||
| type Flags struct { | ||
| KubeConfigPath string `json:"KubeConfigPath"` |
There was a problem hiding this comment.
Is there any value in adding all our configmap params under flags ?
There was a problem hiding this comment.
it makes the signature for start cleaner, passing in config struct and flags struct, when we add more flags in the future
func start(config npmconfig.Config, flags npmconfig.Flags) error {
npm/cmd/main.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| FlagVersion = "version" |
There was a problem hiding this comment.
Can these constants be lower case ?
| klog.Infof("Start NPM version: %s", version) | ||
|
|
||
| var err error | ||
| defer func() { |
There was a problem hiding this comment.
Is there a reason to remove recover ?
There was a problem hiding this comment.
if we have a crash we want to crash hard and bubble that up to orchestrator
npm/cmd/start.go
Outdated
| // Create the kubernetes client | ||
| var k8sConfig *rest.Config | ||
| if flags.KubeConfigPath == "" { | ||
| var err error |
There was a problem hiding this comment.
move this var err outside the if condition?
| @@ -0,0 +1,9 @@ | |||
| $cpEndpoint = Get-Content C:\k\config | ForEach-Object -Process {if($_.Contains("server:")) {$_.Trim().Split()[1]}} | |||
There was a problem hiding this comment.
Can you add some comments on what/how cpendpoint is or looks like, similarly for other fields. Have a readme.md for this logic, if possible
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
add powershell.exe -command "& { . .\windows.ps1; azure-npm-image v26-windows-amd64 }" |
matmerr
force-pushed
the
windowsnpmdaemonset
branch
from
223b16d to
49e971b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
matmerr
force-pushed
the
windowsnpmdaemonset
branch
from
2925669 to
b0717d0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Pull request contains merge conflicts. |
npm.txt
Outdated
| @@ -0,0 +1,59 @@ | |||
| start here | |||
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
npm/cmd/start.go
Outdated
| return fmt.Errorf("failed to generate clientset with cluster config: %w", err) | ||
| } | ||
|
|
||
| klog.Infof("received clientset %+v", clientset) |
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| kind: ClusterRoleBinding |
There was a problem hiding this comment.
< nit > remove the space,
npm/cmd/main.go
Outdated
| var version string | ||
|
|
||
| func main() { | ||
| fmt.Println("start here") |
There was a problem hiding this comment.
we will need to remove these comments in next PR
npm/cmd/main.go
Outdated
| viper.AutomaticEnv() | ||
| initCommandFlags(rootCmd.Commands()) | ||
| }) | ||
| fmt.Println("made it here2") |
There was a problem hiding this comment.
this should be removed too.
vakalapa
left a comment
vakalapa
left a comment
There was a problem hiding this comment.
lgtm just couple of comments for us to apply in next PR
matmerr
force-pushed
the
windowsnpmdaemonset
branch
from
1995358 to
a5f9f67
Compare
2 participants
Reason for Change:
build and deploy windows npm example
Issue Fixed:
Requirements:
Notes: