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

Do not store username/password information in our Image resource. #28

Merged
merged 1 commit into from
Oct 15, 2018

Conversation

CyrusNajmabadi
Copy link
Contributor

Fixes #19

Copy link
Contributor

@swgillespie swgillespie left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@joeduffy joeduffy left a comment

Choose a reason for hiding this comment

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

This feels a little strange. Don't we at least need to republish if the registry info changes?

@CyrusNajmabadi
Copy link
Contributor Author

This feels a little strange. Don't we at least need to republish if the registry info changes?

Can you clarify? how would that affect things? The registry info, right now, is only used for this bit of logic we have:

        const imageData = pulumi.output(args).apply(async (imageArgs) => {
            let localImageName = imageArgs.localImageName;
            if (!localImageName) {
                localImageName = imageArgs.imageName;
            }
            const registry = imageArgs.registry;
            const id = await buildAndPushImageAsync(
                localImageName,
                imageArgs.build,
                imageArgs.imageName,
                this,
                registry && (async () => {
                    return {
                        registry: registry.server,
                        username: registry.username,
                        password: registry.password,
                    };
                }),
            );
            const digest = await getDigest(imageArgs.imageName, this);
            return { digest, id };
        });

In other words, it's only used to be able to login to docker in order to build/push, and so that we can get the docker digest/id.

If they change, we'll still attempt to build/push. But we'll use the results of hte build/push to determine i fhte Image itself has changed. If the digest/id are unchangd, then this Image can still be used. If it is changed, well, then we replace accordingly.

Basically, the registry/username/password are used to compute data that is intrinsic to this resource. But they themselves don't seem to otherwise be part of the resource at all.

@swgillespie
Copy link
Contributor

This feels a little strange. Don't we at least need to republish if the registry info changes?

Since this is a component resource, the code that checks if it needs to republish always runs. We will still republish if a registry info changes. The main change here is that the engine won't show a diff on this component resource whenever the registry changes - it will, though, still publish to the new registry if it needs to.

Component resources are a little strange in that their inputs don't really matter. They're only there to give the engine something to work with when it's diffing and displaying what happened to a component resource. Most steps are no-ops on component resources anyway.

@swgillespie
Copy link
Contributor

In general I think that it almost doesn't make sense to talk about inputs to component resources at all, since it's really the whole program that serves as the input to a component resource.

@CyrusNajmabadi
Copy link
Contributor Author

Ok. Chatted a bunch with @swgillespie . Agreed that ComponentResources are wonky. That's a larger ongoing discussion to have over time. For now, it's a reasonable to have the 'Regsitry' be part of the shape of hte Image, as that info is only used for computation purposes, and is not necessarily that affects the final shape of hte Image object.

@CyrusNajmabadi CyrusNajmabadi merged commit a88ca69 into master Oct 15, 2018
@pulumi-bot pulumi-bot deleted the imageRegistry branch October 15, 2018 22:22
@discovery-NukulSharma
Copy link

How did we able to resolve it we have similar issue?

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.

4 participants