Digdag push validation on the server side.

[hiroyuki-sato]

Hello, @muga , @yoyama

Could you advise me how to implement a validation?

digdag push requires validation on the server side.
It requires two things at least. And this issue will Fix two issues.

• task validation(pushing dig project to server should verify digs #859).
• schedule validation(Invalid cron doesn't return right error message #922).

I implement the first one. Second schedule validation seems to require SchedulerManager class.
hiroyuki-sato@5004fb6#diff-84a9dc43263ac1f78fa3b2aca8234f6fR576

I'm not familiar with Google Guice. I'm not sure how to instantiate it. Do you have any idea for that?

This code almost copies of digdag check code.
https://github.com/treasure-data/digdag/blob/master/digdag-cli/src/main/java/io/digdag/cli/Check.java

Best regards.

[muga]

Hi @hiroyuki-sato, thank you for taking this improvement.

I'm not familiar with Google Guice. I'm not sure how to instantiate it. Do you have any idea for that?

ScheduleManager is already injected in digdag-server ServerModule. Please add the ScheduleManager parameter to the constructor in ProjectResource like WorkflowCompiler. Then, we can use the object in the class without the object creation explicitly. Your code change basically sounds good to me.

[hiroyuki-sato]

@muga Thank you for the advice.

I implement it. I'll add integration tests later.

validate tasks.

digdag push hoge
2019-02-26 16:39:41 +0900: Digdag v0.9.33
Creating .digdag/tmp/archive-5515688681553749096.tar.gz...
  Archiving fuga.dig
  Archiving scripts/myscript.sh
Workflows:
  fuga.dig
error: Status code 400: {"message":"io.digdag.core.repository.ModelValidationException: Validating workflow failed\n+fuga+setup contains invalid keys: 'sh': \"{\"sh\":\"echo start ${session_time}\"}\"","status":400}

validate schedule.

2019-02-26 16:42:19 +0900: Digdag v0.9.33
Creating .digdag/tmp/archive-6443414997521903051.tar.gz...
  Archiving fuga.dig
  Archiving scripts/myscript.sh
Workflows:
  fuga.dig
error: Status code 400: {"message":"hourly>: scheduler requires mm:ss format: test","status":400}

[muga]

@hiroyuki-sato it's cool. thanks!

[hiroyuki-sato]

@muga. Please take a look when you have time.

I created ValidateProjectIT.java for testing this PR.
Is it better to place those tests in another class?

Those comment part doesn't need to this class. I will just remove it.
What do you think?. I'm not sure it possible to validate more.

                        // validate workflow and schedule
//                        Set<String> required = new HashSet<>();
                        for (WorkflowTask task : wf.getTasks()) {
                            // raise an exception if task doesn't valid.
                            task.getConfig();
//                            String require = config.getOptional("require>", String.class).orNull();
//                            if (require != null && required.add(require)) {
//                                f.ln("  -> %s", require);
//                            }

[muga]

@hiroyuki-sato sorry to be late response.

I created ValidateProjectIT.java for testing this PR. Is it better to place those tests in another class?

Good for me to keep this class. Thank you for adding the test.

Those comment part doesn't need to this class. I will just remove it. What do you think?. I'm not sure it possible to validate more.

I believe this is good as first validation. We will be able to refine it later. so, please remove those comment lines.

[muga]

To keep the logic of putProject method simple, can you separate your validation logic from this method and copy the logic to another private method that you will create?

[hiroyuki-sato]

Separated validation method.

[hiroyuki-sato]

@muga Thank you for the comment. I removed unnecessary comments and moved validation into a separate method. Please take a look when you have time.

[muga]

@hiroyuki-sato thank you for improving! Looks good to me. @yoyama what do you think this change??

[yoyama]

@hiroyuki-sato I added a few comments. Could you fix them? Then LGTM.

[hiroyuki-sato]

@yoyama Thank you for the comment. There is still a much-unused import statement. That's why I can't use Optimize imports feature in IntelliJ. 😢
Anyway, I removed those imports which I added.
Please take a look when you have time.

[yoyama]

Thanks! LGTM

[muga]

merged..

[hiroyuki-sato]

Thank you!