Skip to content

feat: prototype control api devloop intent #6636

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

Merged
merged 8 commits into from
Oct 7, 2021

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Sep 24, 2021

This is a prototype for intellij IDE to verify. /cc @emmanuelbaah

Motivation:

  1. IDEs are trying to incorporate control API in their UI. ( see go/ccij-interactive-dd)
  2. When this DD was written, we did not Output logging revamp flushed out. During the implementation, it was discovered when IDEs send build, sync and deploy intent like below, a devloop will be triggered for each build, deploy and sync.
{
  "build": true,
  "sync": true,
  "deploy": true
}

This does not work well with V2 output logging as the output tree gets wiped out at the start of each DevLoop Event.

Solution

  1. Add a new intent devloop however, re-use the build, deploy and sync triggers to execute devloop intent.
    PROs:
    • No additional checks in doDev command for another intent.
  2. When devloop intent is true, then internally set build, deploy and sync is set to true.
  3. Modify the current doDev needsBuild, needsDeploy and needsSync logic to make sure if devloop intent is set to true and rebuild is needed (if build intent was set to true), redeploy. Similarly for sync

Steps to verify

Would love to see if this works and we will try to get it by early oct.

Steps to try out.

  1. Checkout this fork
  2. run make to build the skaffold binary
  3. Point this binary in CC IDE plugin or put it on your system path.
  4. Make a request to re-trigger devloop i.e. build, deploy and sync.
curl --header "Content-Type: application/json" \
  --request POST \
  --data '{"devloop":true}' localhost:50052/v1/execute
  1. Try the new feature!!

Thanks
Tejal

Testcases

  1. Testing case which @briandealwis mentioned
    1. change build dependency & deploy dependency
    2. send build intent.
      Expected Result: Deploy should not happen.
 getting-started git:(add_control_api) ✗ ../../out/skaffold dev -d gcr.io/tejal-test --auto-deploy=false --auto-build=false --auto-sync=false
Listing files to watch...
....

[getting-started] Hello world!!
DEBU[0143] build intent received, calling back to runner  subtask=-1 task=DevLoop
TRAC[0143] dev intents: build true, sync false, deploy false  subtask=-1 task=DevLoop
DEBU[0143]  devloop: build true, sync false, deploy false  subtask=-1 task=DevLoop
Generating tags...
 - skaffold-example -> DEBU[0143] Running command: [git describe --tags --always]  subtask=-1 task=Build

DEBU[0143] Running command: [git status . --porcelain]   subtask=-1 task=Build
DEBU[0143] Command output: [ M examples/getting-started/k8s-pod.yaml
 M examples/getting-started/main.go
?? examples/getting-started/rendered.yaml
]  subtask=-1 task=Build
gcr.io/tejal-gke1/skaffold-example:v1.29.0-145-g2627680df-dirty
INFO[0143] Tags generated in 39.92878ms                  subtask=-1 task=Build
Checking cache...
DEBU[0143] Could not import artifact from Docker, building instead (import of missing images disabled)  subtask=-1 task=Build
 - skaffold-example: Not found. Building
INFO[0143] Cache check completed in 812.37µs             subtask=-1 task=Build
Starting build...
Building [skaffold-example]...
DEBU[0143] Running docker build: context: ., dockerfile: Dockerfile  subtask=skaffold-example task=Build
Sending build context to Docker daemon  3.072kB
Step 1/8 : FROM golang:1.15 as builder
 ---> 40349a2425ef
Step 2/8 : COPY main.go .
 ---> d9a901f14be9
Step 3/8 : ARG SKAFFOLD_GO_GCFLAGS
 ---> Running in 2c0a1beee448
 ---> 53d6160f0200
Step 4/8 : RUN go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -o /app main.go
 ---> Running in 653c2a987675
 ---> 876def090480
Step 5/8 : FROM alpine:3
 ---> 021b3423115f
Step 6/8 : ENV GOTRACEBACK=single
 ---> Using cache
 ---> a730098f0c7d
Step 7/8 : CMD ["./app"]
 ---> Using cache
 ---> 81e70ad197dd
Step 8/8 : COPY --from=builder /app .
 ---> 85c58f013885
Successfully built 85c58f013885
Successfully tagged gcr.io/tejal-gke1/skaffold-example:v1.29.0-145-g2627680df-dirty
The push refers to repository [gcr.io/tejal-gke1/skaffold-example]
39376850b731: Preparing
bc276c40b172: Preparing
bc276c40b172: Layer already exists
39376850b731: Pushed
v1.29.0-145-g2627680df-dirty: digest: sha256:cde265e874af786b7147cd8544c5f3ccb75be709e4f31b53010e6435705b7c8b size: 739
INFO[0154] Build completed in 11.531 seconds             subtask=-1 task=Build
DEBU[0154] push value not present in isImageLocal(), defaulting to true because cluster.PushImages is true  subtask=-1 task=DevLoop
Not watching for changes...
[getting-started] Hello world!


Changed deploy and build dependency

git status examples/getting-started
	modified:   k8s-pod.yaml
	modified:   main.go

@tejal29 tejal29 requested a review from a team as a code owner September 24, 2021 18:17
@google-cla google-cla bot added the cla: yes label Sep 24, 2021
@codecov
Copy link

codecov bot commented Sep 24, 2021

Codecov Report

Merging #6636 (2fcbe74) into main (290280e) will decrease coverage by 0.73%.
The diff coverage is 57.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6636      +/-   ##
==========================================
- Coverage   70.48%   69.75%   -0.74%     
==========================================
  Files         515      523       +8     
  Lines       23150    23876     +726     
==========================================
+ Hits        16317    16654     +337     
- Misses       5776     6132     +356     
- Partials     1057     1090      +33     
Impacted Files Coverage Δ
cmd/skaffold/skaffold.go 0.00% <ø> (ø)
pkg/diag/recommender/container_errors.go 0.00% <0.00%> (ø)
pkg/diag/validator/pod.go 1.32% <0.00%> (ø)
pkg/skaffold/build/buildpacks/logger.go 0.00% <ø> (ø)
pkg/skaffold/build/cluster/logs.go 0.00% <ø> (-16.67%) ⬇️
pkg/skaffold/build/jib/errors.go 48.93% <50.00%> (ø)
cmd/skaffold/app/cmd/cmd.go 70.32% <66.66%> (-0.73%) ⬇️
pkg/diag/validator/resource.go 47.05% <66.66%> (ø)
pkg/skaffold/build/docker/docker.go 86.44% <66.66%> (-2.85%) ⬇️
pkg/skaffold/build/docker/errors.go 63.33% <80.00%> (-2.23%) ⬇️
... and 93 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 509719e...2fcbe74. Read the comment docs.

@tejal29 tejal29 added the kokoro:force-run forces a kokoro re-run on a PR label Sep 24, 2021
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Sep 24, 2021
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea here that this will avoid the separate devloop instances for each of sync/build/deploy?

@tejal29
Copy link
Contributor Author

tejal29 commented Sep 27, 2021

Is the idea here that this will avoid the separate devloop instances for each of sync/build/deploy?

yes sorry. I didn't add any description and put this out late friday. Will add motivation to the PR

@emmanuelbaah
Copy link

Tested and verified!

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Oct 5, 2021
@tejal29
Copy link
Contributor Author

tejal29 commented Oct 5, 2021

Thanks for the thorough testing @marlon-gamez. Added integration tests to make sure the case you discovered never happens.

Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a few questions regarding the integration test

Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, changes look good so I think it's good to merge once tests are good

@gsquared94 gsquared94 merged commit 906ab1e into GoogleContainerTools:main Oct 7, 2021
@tejal29 tejal29 deleted the add_control_api branch November 9, 2021 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants