Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

- `dr workload up` now deploys onto an errored workload instead of refusing it, whenever the deploy gives it something new to run: a code change, a change to the manifest or the sizing, or `--force-build`. What the workload is running has failed, so there is nothing serving for the swap to endanger, and the plan says so with the platform's own reason for the failure beside the state. A deploy with nothing new in it is still refused, and the refusal names what would work: `--force-build` for a project whose image the platform builds, a change to the image or artifact otherwise, and the delete as the last resort. An errored workload never has its image copied forward, since on a locked version the copy would be locked too, permanently, pointing at an image the registry may no longer have; the version is built instead. A stopped workload whose start comes up errored ahead of a roll is rolled anyway, and the name conflict on an errored holder advises binding to it again. Together these close a deadlock in which a locked production workload whose image the registry had pruned could be neither deployed onto, nor started, nor rebuilt.
- `dr workload up --force-build` now rebuilds and rolls even when the working tree and the manifest have not changed, which is the case the flag exists for: an image gone from the registry leaves both exactly as they were. The flag used to be read only by the build step, so a plan with nothing else in it came out empty and the run reported the workload as up to date. A forced build on a locked version locks its successor to match; on a manifest that names its image rather than building it the flag stays idle and says so.
- `dr artifact code sync` and `dr workload up` now replace files in the catalog when the upload takes the zip route, which is any change set of more than 20 files or 50 MB. The overwrite mode was sent only as a query parameter of the Files API's `fromFile` upload, which the server accepts and ignores, so its rename default applied: every path already in the catalog came back as a `name (2).ext` duplicate holding the new bytes while the original kept the old ones, the version had twice the files, and an image built from it ran stale code next to junk. Smaller change sets, which go through a stage, were never affected. The mode now travels in the multipart form, ahead of the file, where the server reads it; it is still sent in the query as well until the API says which of the two is authoritative.
- `dr workload up` now says when `.env` and the manifest have parted, instead of reporting a deploy as up to date without having looked at the file you just edited. Nothing is applied and nothing is written: a deploy stays a function of the committed repo, and reading the file to say the two disagree is not deploying from it. The notice is the one `dr workload config` has printed all along, names only and never values, and it names the flag that settles it on the command you are already running rather than sending you to a different one, carrying the `--dir` this run was given because `config` looks only where it is pointed while `up` walks upward for the manifest. Silent where there is no `.env`, which is the ordinary CI case, and silent where the two files agree. What is left to `config` is everything that is true of the project rather than of this run: the values behind credential references, which nothing can compare; the names the classifier read as local-only, which no flag will ever add; an entry left naming the credential placeholder, which the deploy refuses for itself when it reaches it; and a manifest whose shape no flag can edit, whose refusal counts the whole of `.env` as missing. None of those can be settled by anything the reader is about to run, so on a deploy each would print on every run for the life of the project, and a line that always prints is one you stop reading along with the drift beside it.
- `dr workload up --update-env` and `dr workload config --update-env` now name the `.env` variables the manifest does not declare even when the run rewrote something. The notice used to arrive only on the run that found nothing to do, so reconciling every value you could left the file reading as a clean bill of health while a variable it had never carried went on reaching nothing.
- `dr artifact code sync` no longer overwrites or deletes a local file without a recoverable copy. Before the remote wins, your version is saved as `<path>.LOCAL.<timestamp>`, for every case that touches the working tree — not only conflicts, as before — including a remote-modified download that used to be applied silently with no backup. Those `.LOCAL` copies are also excluded from the next sync, so a backup is never re-uploaded as new content.
Expand Down
88 changes: 88 additions & 0 deletions internal/drapi/filesapi/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"bytes"
"encoding/json"
"io"
"mime"
"mime/multipart"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -364,6 +365,12 @@ func TestPollStatus_CompletedRedirect(t *testing.T) {
assert.True(t, IsTerminalStatus(resp.Status))
}

// TestUploadFromZipExisting pins where the overwrite mode travels. The
// Files API reads it from the multipart form: a value sent only in the
// query is accepted and ignored, the rename default applies, and every
// existing path comes back as "name (2).ext". So the form field must be
// present and must precede the file part. The query copy stays until the
// API documents which location is authoritative.
func TestUploadFromZipExisting(t *testing.T) {
startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "/api/v2/files/cid-1/fromFile/", r.URL.Path)
Expand All @@ -381,9 +388,29 @@ func TestUploadFromZipExisting(t *testing.T) {
return
}

assert.Equal(t, "overwrite", part.FormName(), "form fields must precede the file part")
assert.Empty(t, part.FileName())

value, err := io.ReadAll(part)
assert.NoError(t, err)
assert.Equal(t, "REPLACE", string(value))

part, err = mr.NextPart()
if !assert.NoError(t, err) {
return
}

assert.Equal(t, "file", part.FormName())
assert.Equal(t, "changes.zip", part.FileName())

zipBytes, err := io.ReadAll(part)
assert.NoError(t, err)
assert.Equal(t, "PK\x03\x04fake-zip", string(zipBytes))

// Expect no further parts.
_, err = mr.NextPart()
assert.ErrorIs(t, err, io.EOF)

w.WriteHeader(http.StatusAccepted)
_, _ = w.Write([]byte(`{"catalogId":"cid-1","catalogVersionId":"v9","statusId":"sid-9"}`))
}))
Expand All @@ -397,6 +424,62 @@ func TestUploadFromZipExisting(t *testing.T) {
assert.Equal(t, "sid-9", resp.StatusID)
}

// TestUploadFromZipExisting_ContentLengthWithFormFields checks that folding
// form fields into the prologue keeps the advertised Content-Length exact
// and the streamed file intact. An off-by-N here surfaces as a transport
// error or a truncated part, never as a silent pass.
func TestUploadFromZipExisting_ContentLengthWithFormFields(t *testing.T) {
// Long enough to cross io.Copy's internal buffer more than once.
payload := strings.Repeat("0123456789abcdef", 4096)

startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
raw, err := io.ReadAll(r.Body)
if !assert.NoError(t, err) {
return
}

assert.Equal(t, r.ContentLength, int64(len(raw)),
"advertised Content-Length must match the received body")

// The body is consumed above, so parse the buffered copy.
_, params, err := mime.ParseMediaType(r.Header.Get("Content-Type"))
if !assert.NoError(t, err) {
return
}

mr := multipart.NewReader(bytes.NewReader(raw), params["boundary"])

part, err := mr.NextPart()
if !assert.NoError(t, err) {
return
}

value, err := io.ReadAll(part)
assert.NoError(t, err)
assert.Equal(t, "overwrite", part.FormName())
assert.Equal(t, "REPLACE", string(value))

part, err = mr.NextPart()
if !assert.NoError(t, err) {
return
}

got, err := io.ReadAll(part)
assert.NoError(t, err)
assert.Equal(t, "file", part.FormName())
assert.Equal(t, payload, string(got))

w.WriteHeader(http.StatusAccepted)
_, _ = w.Write([]byte(`{"catalogId":"cid-1","catalogVersionId":"v9","statusId":"sid-9"}`))
}))

c := New()

body := strings.NewReader(payload)
_, err := c.UploadFromZipExisting("cid-1", "changes.zip", OverwriteReplace, int64(body.Len()), body)
require.NoError(t, err)
}

// TestUploadFromZipNew_HitsFromFileEndpoint locks in the (post-2026-04-30)
// fix that the new-catalog-from-zip path posts to /files/fromFile/ rather
// than /files/. The bare /files/ endpoint silently created an empty catalog
Expand All @@ -421,6 +504,11 @@ func TestUploadFromZipNew_HitsFromFileEndpoint(t *testing.T) {
assert.Equal(t, "file", part.FormName())
assert.Equal(t, "wapi-sync.zip", part.FileName())

// A new catalog has no paths to collide with, so no overwrite
// field travels: the file is the only part.
_, err = mr.NextPart()
assert.ErrorIs(t, err, io.EOF)

w.WriteHeader(http.StatusAccepted)
_, _ = w.Write([]byte(`{"catalogId":"new-cid","catalogVersionId":"new-ver","statusId":"sid-new"}`))
}))
Expand Down
20 changes: 16 additions & 4 deletions internal/drapi/filesapi/fromfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,18 @@ func (c *httpClient) UploadFromZipNew(name string, size int64, body io.Reader) (
return nil, fmt.Errorf("build files url: %w", err)
}

return uploadZipMultipart(requestURL, name, size, body)
return uploadZipMultipart(requestURL, nil, name, size, body)
}

// UploadFromZipExisting adds a zip's contents to catalogID as a new version.
//
// The overwrite mode is sent both as a multipart form field and as a query
// parameter. The Files API reads it from the form: a value sent only in
// the query is accepted and ignored, the server default (rename) applies,
// and every path already in the catalog comes back as a "name (2).ext"
// duplicate while the original keeps its old bytes. The contract does not
// say which location is authoritative, so the query copy stays until it
// does; the form field is the one that takes effect today.
func (c *httpClient) UploadFromZipExisting(catalogID, name, overwrite string, size int64, body io.Reader) (*FromFileResp, error) {
if overwrite == "" {
overwrite = OverwriteReplace
Expand All @@ -45,16 +54,19 @@ func (c *httpClient) UploadFromZipExisting(catalogID, name, overwrite string, si
q.Set("useArchiveContents", "true")
q.Set("overwrite", overwrite)

fields := url.Values{}
fields.Set("overwrite", overwrite)

requestURL, err := drapi.EndpointURL("/files/"+url.PathEscape(catalogID)+"/fromFile/", q)
if err != nil {
return nil, fmt.Errorf("build fromFile url: %w", err)
}

return uploadZipMultipart(requestURL, name, size, body)
return uploadZipMultipart(requestURL, fields, name, size, body)
}

func uploadZipMultipart(requestURL, name string, size int64, body io.Reader) (*FromFileResp, error) {
req, err := newStreamingMultipartRequest(requestURL, nil, name, size, body)
func uploadZipMultipart(requestURL string, fields url.Values, name string, size int64, body io.Reader) (*FromFileResp, error) {
req, err := newStreamingMultipartRequest(requestURL, fields, name, size, body)
if err != nil {
return nil, err
}
Expand Down
48 changes: 39 additions & 9 deletions internal/drapi/filesapi/multipart.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net/http"
"net/textproto"
"net/url"
"sort"

"github.com/datarobot/cli/internal/drapi"
)
Expand All @@ -34,22 +35,31 @@ const multipartFormField = "file"
// by the pipe (one chunk in flight) plus the small envelope, regardless
// of file size — important because the engine may upload multi-GiB zips.
//
// fields are written as ordinary form parts ahead of the file part.
// The Files API binds a POST's parameters from the parsed body alone
// and drops unrecognized query parameters without complaining, so an
// option that has to reach the server travels here and not in the URL.
//
// useArchiveContents on the fromFile routes reads like a counter-example
// and is not one. It is sent in the query, discarded there like anything
// else, and extraction still happens only because the server's declared
// form default for that field is already true. It is inert rather than
// honoured, so it says nothing about the query being a usable channel,
// and a flip of that default would stop extraction with no error.
// Moving it into the form is a separate change.
//
// Trade-off: the request has no GetBody, so http.Transport cannot
// transparently retry the body on connection reset. Callers needing
// retry must redo the call from scratch (re-opening the source if it
// isn't seekable).
func newStreamingMultipartRequest(
requestURL string,
query url.Values,
fields url.Values,
Comment thread
wojtekwdr marked this conversation as resolved.
filename string,
size int64,
body io.Reader,
) (*http.Request, error) {
if len(query) > 0 {
requestURL += "?" + query.Encode()
}

contentType, prologue, epilogue, err := multipartFraming(filename)
contentType, prologue, epilogue, err := multipartFraming(fields, filename)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -81,13 +91,33 @@ func newStreamingMultipartRequest(
}

// multipartFraming returns the prologue and epilogue around a single
// file part. Going through multipart.Writer keeps the framing
// RFC-2046-correct even though we stream the body separately.
func multipartFraming(filename string) (string, []byte, []byte, error) {
// file part, with fields framed as complete parts before it. Fields go
// first so a server that parses the stream incrementally has every
// parameter in hand before it commits to reading an arbitrarily large
// file. Names are sorted to keep the framing deterministic. Going through
// multipart.Writer keeps the framing RFC-2046-correct even though we
// stream the body separately.
func multipartFraming(fields url.Values, filename string) (string, []byte, []byte, error) {
var head bytes.Buffer

w := multipart.NewWriter(&head)

names := make([]string, 0, len(fields))

for name := range fields {
names = append(names, name)
}

sort.Strings(names)

for _, name := range names {
for _, value := range fields[name] {
if err := w.WriteField(name, value); err != nil {
return "", nil, nil, fmt.Errorf("write multipart field %s: %w", name, err)
}
}
}

hdr := make(textproto.MIMEHeader)
hdr.Set("Content-Disposition", fmt.Sprintf(`form-data; name=%q; filename=%q`, multipartFormField, filename))
hdr.Set("Content-Type", "application/octet-stream")
Expand Down
Loading