FAAS-7483: Support creating OCIR repos in the non-root compartment. #642

parthasarathy-menon · 2022-01-17T16:58:02Z

Fn context, oracle.image-compartment-id is used to specify the compartment id to which the function image is to be deployed.

If the the repository already exists and oracle.image-compartment-id is specified, the deployment will fail if the repository is not present the same compartment id as the image-compartment-id.

There are no tests added to the code due to the lack of testing around the deploy path, in order to write a test, we would have needed to mock multiple clients and change some fundamental properties in the providers code. Instead, we have performed manual testing:

oracle.image-compartment-Id set in fn context
handling of oracle.image-compartment-Id not set (check in the code early on and skips if not set)
setting oracle.image-compartment-Id to COMPA with a repository/image in COMPB - code will error telling use the repo already exists in another compartment

ahjones · 2022-01-19T13:59:57Z

commands/deploy.go

+func getRepositoryNamePrefix(registry string) (string, error) {
+	repositoryNamePrefix := ""
+	parts := strings.Split(registry, "/")
+	if len(parts) < 3 {
+		return "", errors.New("registry must have a dockerhub owner or private registry and repo-name")
+	}
+	for i := 2; i < len(parts); i++ {
+		repositoryNamePrefix += parts[i] + "/"
+	}
+	return repositoryNamePrefix[:len(repositoryNamePrefix)-1], nil
+}


Is this the same? Could you add some tests too, to be sure, please?

Suggested change

func getRepositoryNamePrefix(registry string) (string, error) {

repositoryNamePrefix := ""

parts := strings.Split(registry, "/")

if len(parts) < 3 {

return "", errors.New("registry must have a dockerhub owner or private registry and repo-name")

}

for i := 2; i < len(parts); i++ {

repositoryNamePrefix += parts[i] + "/"

}

return repositoryNamePrefix[:len(repositoryNamePrefix)-1], nil

}

func getRepositoryNamePrefix(registry string) (string, error) {

parts := strings.Split(registry, "/")

if len(parts) < 3 {

return "", errors.New("registry must have a dockerhub owner or private registry and repo-name")

}

return strings.Join(parts[2:], "/"), nil

}

ahjones · 2022-01-19T14:05:50Z

commands/deploy.go

+		artifactsClient.SetRegion(getRegion(oracleProvider))
+
+		// Create the repository in ImageCompartmentID
+		repositoryExists, err := createContainerRepositoryInCompartment(repositoryName, oracleProvider.ImageCompartmentID, artifactsClient)


I wonder if the logic in this area might be more clear if it was switched around slightly?

What do you think about checking for existence of the container first, with doesRepositoryExistInComparment. If that's true, then you're done. If false, then proceed to create the repository, and if that fails, you know it's a failure that you can't continue with.

That way the response from createContainerRepositoryInCompartment doesn't have to distinguish between 409s and other responses.

Nice catch. Updating it that way

ahjones · 2022-01-19T14:10:13Z

commands/deploy.go

+		CompartmentId: &compartmentID,
+		DisplayName:   &repositoryName})
+	if err != nil {
+		return false, fmt.Errorf("failed to lookup Container Repository due to %s", err)


Suggested change

return false, fmt.Errorf("failed to lookup Container Repository due to %s", err)

return false, fmt.Errorf("failed to lookup Container Repository due to %w", err)

commands/deploy.go

+		}
+		if !repositoryExists {
+			createContainerRepositoryInCompartment(repositoryName, oracleProvider.ImageCompartmentID, artifactsClient)
+			if err != nil {


ahjones · 2022-01-20T10:24:54Z

commands/deploy.go

+	if oracleProvider != nil && oracleProvider.ImageCompartmentID != "" {
+		// If the provider is Oracle and ImageCompartmentID is present, we need to deploy image to the ImageCompartmentID.
+
+		// Check Registry and build the repository name.


nit: most of these small comments aren't doing a lot for me. e.g., here "check registry and build the repository name" just says exactly what the code is doing. Personally I think it would be better (but not required) to write a small comment at the top describing roughly /why/ things are happening the way they are. e.g., "A repository must have a unique name within the tenancy. If one doesn't exist create it in the compartment; if one exists already check to see if it is in the right compartment. If it's not in the right compartment we can't continue".

Here the comments I am leaving are like steps and the code is what the step does. Its much easier to understand what is happening later on when reading than to see a chunk of code and figure out what it does, especially for someone new. I'll add why we do it at the top and space it so it helps in a similar way.

ahjones · 2022-01-20T12:31:58Z

client/api.go

@@ -26,3 +26,7 @@ import (
 func CurrentProvider() (provider.Provider, error) {
 	return fn_go.DefaultProviders.ProviderFromConfig(viper.GetString(config.ContextProvider), &config.ViperConfigSource{}, &provider.TerminalPassPhraseSource{})
 }
+
+func Registry() string {


I don't think that Registry should be added to api.go.

ahjones · 2022-01-20T12:34:06Z

commands/deploy.go

+		if client.Registry() == "" {
+			return errors.New("registry name must be set, be sure to configure your context file")
+		}
+		res, err := getRepositoryNamePrefix(client.Registry())


I think that the existing function getRepositoryName will do what you want here. Additionally it'll support name containing a different registry

Suggested change

res, err := getRepositoryNamePrefix(client.Registry())

res, err := getRepositoryName(funcfile)

ahjones · 2022-01-20T12:34:57Z

commands/deploy.go

@@ -697,3 +729,36 @@ func findMissingValues(signingDetails common.SigningDetails) string {
 	}
 	return strings.Join(missingValues, ",")
 }
+
+func getRepositoryNamePrefix(registry string) (string, error) {


I'm not sure that this fn is needed, if you use getRepositoryName as suggested elsewhere.

ahjones · 2022-01-20T12:35:29Z

commands/deploy.go

+		CompartmentId: &compartmentID,
+		DisplayName:   &repositoryName})
+	if err != nil {
+		return false, fmt.Errorf("failed to lookup Container Repository due to %w", err)


Suggested change

return false, fmt.Errorf("failed to lookup Container Repository due to %w", err)

return false, fmt.Errorf("failed to lookup container repository due to %w", err)

ahjones · 2022-01-20T12:36:49Z

commands/deploy.go

+	if err != nil {
+		return false, fmt.Errorf("failed to lookup Container Repository due to %w", err)
+	}
+	if *response.RepositoryCount == 1 {


I'm not sure about this suggestion, but it seems that if the count is bigger than one it exists.

Suggested change

if *response.RepositoryCount == 1 {

if *response.RepositoryCount >= 1 {

JadeRedworth force-pushed the FAAS-7483/image-compartment-support branch 2 times, most recently from 6506436 to cbcdc51 Compare January 17, 2022 17:32

parthasarathy-menon force-pushed the FAAS-7483/image-compartment-support branch 2 times, most recently from 1247e42 to 0918865 Compare January 18, 2022 15:36

ahjones reviewed Jan 19, 2022

View reviewed changes

parthasarathy-menon force-pushed the FAAS-7483/image-compartment-support branch 2 times, most recently from 6bdc9b4 to 46dde9d Compare January 20, 2022 10:30

ahjones reviewed Jan 20, 2022

View reviewed changes

parthasarathy-menon force-pushed the FAAS-7483/image-compartment-support branch from 46dde9d to 8e30bf9 Compare January 20, 2022 10:51

ahjones reviewed Jan 20, 2022

View reviewed changes

Support creating OCIR repos in the non-root compartment

e1e54c6

parthasarathy-menon force-pushed the FAAS-7483/image-compartment-support branch from 8e30bf9 to e1e54c6 Compare January 20, 2022 12:51

ahjones approved these changes Jan 20, 2022

View reviewed changes

riconnon approved these changes Jan 20, 2022

View reviewed changes

riconnon merged commit 1782eed into master Jan 20, 2022

riconnon deleted the FAAS-7483/image-compartment-support branch January 20, 2022 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FAAS-7483: Support creating OCIR repos in the non-root compartment. #642

FAAS-7483: Support creating OCIR repos in the non-root compartment. #642

parthasarathy-menon commented Jan 17, 2022 •

edited by JadeRedworth

Loading

ahjones Jan 19, 2022

ahjones Jan 19, 2022

parthasarathy-menon Jan 19, 2022

ahjones Jan 19, 2022

This comment was marked as resolved.

This comment was marked as resolved.

ahjones Jan 20, 2022

parthasarathy-menon Jan 20, 2022

ahjones Jan 20, 2022

ahjones Jan 20, 2022

ahjones Jan 20, 2022

ahjones Jan 20, 2022

ahjones Jan 20, 2022

	return false, fmt.Errorf("failed to lookup Container Repository due to %s", err)
	return false, fmt.Errorf("failed to lookup Container Repository due to %w", err)

	res, err := getRepositoryNamePrefix(client.Registry())
	res, err := getRepositoryName(funcfile)

	if *response.RepositoryCount == 1 {
	if *response.RepositoryCount >= 1 {

FAAS-7483: Support creating OCIR repos in the non-root compartment. #642

FAAS-7483: Support creating OCIR repos in the non-root compartment. #642

Conversation

parthasarathy-menon commented Jan 17, 2022 • edited by JadeRedworth Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthasarathy-menon commented Jan 17, 2022 •

edited by JadeRedworth

Loading