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

[SNOW-559751] Add role switching to SimpleIngestManager #156

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sfc-gh-kbregula
Copy link
Collaborator

Copy link
Contributor

@sfc-gh-tzhang sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

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

LGTM!

* @param hostName the hostname
* @param keyPair keyPair associated with the private key used for authentication. See @see {@link
* Utils#createKeyPairFromPrivateKey} to generate KP from p8Key
* @param userAgentSuffix user agent suffix we want to add.
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comment for role

@@ -388,6 +440,28 @@ public SimpleIngestManager(
new RequestBuilder(account, user, keyPair, schemeName, hostName, port, userAgentSuffix);
}

/* Another flavor of constructor which supports userAgentSuffix and role */
Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc

* RequestBuilder constructor which uses default schemes, host and port.
*
* @param accountName - the name of the Snowflake account to which we're connecting
* @param userName - the username of the entity loading files
Copy link
Contributor

Choose a reason for hiding this comment

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

missing hostName

assertEquals(0L, configureClientResponse.getClientSequencer().longValue());
}

@Ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

why these tests are ignored for now? Could you add a JIRA for enabling them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests are disabled from the beginning as they arose. I don't know what the story is behind this.
https://github.com/snowflakedb/snowflake-ingest-java/pull/105/files

Copy link
Contributor

Choose a reason for hiding this comment

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

@sfc-gh-japatel i think it could be enabled now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have it enabled already but I forgot to push.

@sfc-gh-kbregula sfc-gh-kbregula changed the title [SNOW-559751] Add role swithcing to SimpleIngestManager [SNOW-559751] Add role swktc to SimpleIngestManager Apr 14, 2022
@sfc-gh-kbregula sfc-gh-kbregula changed the title [SNOW-559751] Add role swktc to SimpleIngestManager [SNOW-559751] Add role switching to SimpleIngestManager Apr 14, 2022
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.

3 participants