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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new docker auth settings to truss config. #876

Merged
merged 6 commits into from Mar 26, 2024

Conversation

squidarth
Copy link
Collaborator

@squidarth squidarth commented Mar 24, 2024

See https://github.com/basetenlabs/baseten/pull/8198 for all of the context here, as well as for how I really tested.

馃殌 What

I would love feedback on the naming here / user interface. Currently, I've defined two auth methods:

  • GCS_SERVICE_ACCOUNT_JSON_BASE64
  • ACCESS_TOKEN

Do these seem coherent?

馃捇 How

馃敩 Testing

Again, see https://github.com/basetenlabs/baseten/pull/8198 for the full, gory details.

@squidarth squidarth requested a review from bolasim March 25, 2024 15:11
@squidarth squidarth marked this pull request as ready for review March 25, 2024 15:12
@@ -297,19 +297,67 @@ def to_list(self) -> List[Dict[str, str]]:
return [item.to_dict() for item in self.items]


class DockerAuthType(Enum):
GCS_SERVICE_ACCOUNT_JSON_BASE64 = "GCS_SERVICE_ACCOUNT_JSON_BASE64"
ACCESS_TOKEN = "ACCESS_TOKEN"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The access token is also technically, username:access_token | base64. If you're going to write the encoding in the name, maybe it's worth making that explicit here?


@dataclass
class DockerAuthSettings:
""" "
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: extra "

truss/truss_config.py Show resolved Hide resolved
truss/truss_config.py Show resolved Hide resolved
Copy link
Collaborator

@bolasim bolasim left a comment

Choose a reason for hiding this comment

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

Should we consider adding light docs to this PR?

@squidarth
Copy link
Collaborator Author

Should we consider adding light docs to this PR?

@bolasim I'll add some docs after the backend changes go out

@squidarth squidarth merged commit f2c36dc into main Mar 26, 2024
3 checks passed
@squidarth squidarth deleted the sshanker/add-new-docker-auth-settings branch March 26, 2024 15:41
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.

None yet

2 participants