Unix sockets to port communication #54
Conversation
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
+ Coverage 56.13% 56.16% +0.03%
==========================================
Files 16 16
Lines 1418 1419 +1
==========================================
+ Hits 796 797 +1
Misses 529 529
Partials 93 93
Continue to review full report at Codecov.
|
|
Can you put it behind a feature flag of it's breaking everyone? And the flag is off by default and on for anyone opting in? Would it make sense? |
|
@Skarlso I think I can do that. I just think when we wait longer more users will be pissed at the end. I think now is the perfect time to do changes like these when everyone expects bugs/problems. 😄 |
43fcbd6 to
17b7873
Compare
|
@michelvocks Agreed. :D |
228a34b to
7fd6bbe
Compare
|
This is now working as intended and make Gaia a bit more secure and flexibe. 🎉 🎊 |
7fd6bbe to
765e03d
Compare
|
@michelvocks Yo what's up with this guy? :) The corresponding 58 was merged. :) |
|
@Skarlso if I merge this, it's not possible to build pipelines anymore when you are running gaia from master branch. It will work again if I merge this: gaia-pipeline/gosdk#1 But then it's not possible to build pipelines in version V0.1.2 which is currently latest released version. Maybe it's fine to just merge this and if someone uses master and can just tell him to use my fork. This should actually work. |
deeeb3b to
ee4dc9a
Compare
|
Ping @Skarlso please have a look and merge if you think so 🤗 |
|
I fixed the merge conflict. 👍 Hope you don't mind. :) |
|
@michelvocks Race error on the new scheduler test here: err = os.Remove(filepath.Join(os.TempDir(), "gaia.db")):( |
|
Oh I see why... Interesting. why are we removing a temp directory? I'll fix it. :) EDIT: Ah, I see why. I'll put it into a defer. |
|
Interesting. Something new introduced a race condition after merging master. :/ |
|
I tested this on master and it looks okay. So it's this new code plus the new schedule test that's failing. |
428b4bc to
aeb27ae
Compare
aeb27ae to
9e5610e
Compare
|
@Skarlso I fixed it. Somehow the init method was still in the specific test. No clue what happened there. 😄 |
|
Well done! Awesome.😁 |
| func (p *PluginFake) Connect(cmd *exec.Cmd, logPath *string) error { return nil } | ||
| func (p *PluginFake) Execute(j *gaia.Job) error { return nil } | ||
| func (p *PluginFake) GetJobs() ([]gaia.Job, error) { return prepareJobs(), nil } | ||
| func (p *PluginFake) Close() {} | ||
|
|
||
| type CAFake struct{} | ||
|
|
||
| func (c *CAFake) CreateSignedCert() (string, string, error) { return "", "", nil } |
There was a problem hiding this comment.
Okay, so I know that this will be annoying, but... :D Could you please change the API of the CA to named values in the return statement? Because it's annoying that I don't know what "", "", nil is actually returning by looking at the pop up of the parameters. :D I don't which is which here. Path, or Cert or KeyPath or whatever, you see what I'm saying?
| @@ -36,7 +45,9 @@ func TestInit(t *testing.T) { | |||
| if err := storeInstance.Init(); err != nil { | |||
| t.Fatal(err) | |||
| } | |||
| s := NewScheduler(storeInstance, &PluginFake{}) | |||
| var ca security.CAAPI | |||
There was a problem hiding this comment.
you can remove this variable and change the below ca = &CAFake{} to simply ca := &CAFake{} and all will be well. Do this everywhere. :)
Fixes #53.
This will be a breaking change. Once this is merged we have to update the SDK immediately otherwise create pipeline will fail.
For now implemented first version of CA generation and key pair generation which will be signed for mutual Auth.