Skip to content
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

Removes out-of-scope fields from Connect API request #206

Merged
merged 8 commits into from
Oct 10, 2023
Merged

Conversation

tdstein
Copy link
Collaborator

@tdstein tdstein commented Sep 8, 2023

Intent

The removed fields are out-of-scope of the current implementation.

Closes #192

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change

Approach

Automated Tests

Directions for Reviewers

Checklist

@tdstein
Copy link
Collaborator Author

tdstein commented Sep 8, 2023

@mmarchetti - is this what you were thinking?

@tdstein tdstein self-assigned this Sep 8, 2023
@mmarchetti
Copy link
Contributor

I don't think our Optional type will be considered empty by the JSON encoder when its Valid field is false. The json docs say: false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

@tdstein
Copy link
Collaborator Author

tdstein commented Sep 8, 2023

I don't think our Optional type will be considered empty by the JSON encoder when its Valid field is false. The json docs say: false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

Makes sense. I just wanted to validate that this is the correct approach for the issue.

@mmarchetti
Copy link
Contributor

There are some go proposals to handle this (e.g. golang/go#52803, golang/go#50480). So far they haven't gone anywhere.

@tdstein tdstein changed the title Sets omitempty on optional fields. Removes out-of-scope fields from Connect API request Sep 19, 2023
@tdstein tdstein marked this pull request as ready for review September 25, 2023 19:35
Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

Can we adjust the ConnectContent type in the frontend along with this to keep all of our type parity?

I'd be happy to help with any of the TypeScript.

@tdstein
Copy link
Collaborator Author

tdstein commented Oct 6, 2023

Can we adjust the ConnectContent type in the frontend along with this to keep all of our type parity?

I'd be happy to help with any of the TypeScript.

@dotNomad - done

@tdstein tdstein requested a review from dotNomad October 10, 2023 13:38
@kgartland-rstudio
Copy link
Collaborator

Sorry for the delay here. I tried to deploy to the dogfood server with this build and hit the following error.

{"time":"2023-10-10T13:14:17.718346-04:00","type":"agent/log","data":{"level":"INFO","message":"Adding file","localId":"Xb-ubvViYDtv0XD0","path":"/Users/kgartland/work/publishing-client/test/sample-content/fastapi-simple/requirements.in","size":124}}
{"time":"2023-10-10T13:14:17.718736-04:00","type":"agent/log","data":{"level":"INFO","message":"Bundle created","files":4,"localId":"Xb-ubvViYDtv0XD0","totalBytes":791}}
{"time":"2023-10-10T13:14:17.719064-04:00","type":"publish/createBundle/success","data":{"level":"INFO","message":"Done","filename":"/var/folders/qn/0zx6630d3b15_6jcdyl_20dc0000gp/T/bundle-660687857.tar.gz","localId":"Xb-ubvViYDtv0XD0"}}
{"time":"2023-10-10T13:14:17.719099-04:00","type":"publish/createDeployment/start","data":{"level":"INFO","message":"Creating deployment","localId":"Xb-ubvViYDtv0XD0"}}
{"time":"2023-10-10T13:14:17.854246-04:00","type":"publish/createDeployment/failure","data":{"level":"ERROR","message":"unexpected response from the server","method":"POST","status":400,"url":"http://44.206.227.208/__api__/v1/content","code":121,"error":"The request JSON cannot be parsed: json: unknown field "AccessType"","localId":"Xb-ubvViYDtv0XD0","payload":null},"error":"serverErr"}

Command:
./bin/darwin-arm64/connect-client publish-ui --listen=localhost:9000 ./test/sample-content/fastapi-simple/ -n dogfood

version=f2b70b5

@tdstein
Copy link
Collaborator Author

tdstein commented Oct 10, 2023

Sorry for the delay here. I tried to deploy to the dogfood server with this build and hit the following error.

@kgartland-rstudio - should be resolved now.

Copy link
Collaborator

@kgartland-rstudio kgartland-rstudio left a comment

Choose a reason for hiding this comment

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

Verified fix. Tested on Connect 2022.04.0 (oldest supported version) and Dogfood, both deployed successfully.

@tdstein tdstein merged commit 782f037 into main Oct 10, 2023
13 checks passed
@tdstein tdstein deleted the tdstein/192 branch October 10, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publishing to a version of Connect pre-2023.05 errors
4 participants