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

feat(plugins): use an init container to download plugin resources #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

link108
Copy link
Member

@link108 link108 commented Oct 21, 2019

This is the PR we sent to OSS, I'm going to leave comments on this to give context on what is going on. I'll create another PR (probably will be a WIP) that strips out un-necessary pieces like we discussed today (eg having deck just link to the resources necessary vs serving the resources itself)

@link108
Copy link
Member Author

link108 commented Oct 21, 2019

note: this is not to be merged

public Map<String, Object> options;
public Map<String, List<String>> resources;
Copy link
Member Author

Choose a reason for hiding this comment

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

this change is related to updating the manifest.yml to handle deploying resources to all services. That's done by updating the plugin's manifest.yml to include a resources section instead of listing jars that go on java services. This is not a super necessary change, we could instead add a key to the manifest.yml that includes deck as a key and its values would be js files for Deck to consume


@Override
public String getNodeName() {
return name;
}

public Manifest generateManifest() {
public Manifest getManifest() {
Copy link
Member Author

Choose a reason for hiding this comment

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

this change is related to downloading plugin manifests once. Since this is not necessary, lets skip doing this. Also, this is kind of ok if you spin up a new instance of halyard each time you want to deploy, but if it's a long running halyard process, downloading the manifest once will lead to stale manifests.

*
*/

package com.netflix.spinnaker.halyard.core.resource.v1;
Copy link
Member Author

Choose a reason for hiding this comment

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

this set of changes is what we do need, we'll pass structured data to Deck via settings.js that includes plugin name, resource location, and probably types of resources

import lombok.AccessLevel;
import lombok.Getter;

public class ObjectResource extends ObjectReplaceTemplatedResource {
Copy link
Member Author

Choose a reason for hiding this comment

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

This extends the ObjectReplaceTemplatedResource class above, not entirely sure this class is necessary as contents can be part of the above class, would just have to make it not abstract. I matched the implementation of the StringReplaceTemplatedResource


import spock.lang.Specification

class ObjectReplaceTemplatedResourceSpec extends Specification {
Copy link
Member Author

@link108 link108 Oct 21, 2019

Choose a reason for hiding this comment

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

tests for above, just realized we don't have tests that actually test the replace occurs

DeploymentConfiguration deploymentConfiguration,
SpinnakerRuntimeSettings endpoints) {

public Profile getProfileByName(
Copy link
Member Author

Choose a reason for hiding this comment

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

this change is related to updating the manifest.yml to handle deploying resources to all services. if we don't update manifest.yml to support all services, we don't have to do this! 👍

@@ -232,8 +234,35 @@ protected void setProfile(
bindings.put("canary.showAllCanaryConfigs", canary.isShowAllConfigsEnabled());
}

// Configure Plugins
Copy link
Member Author

Choose a reason for hiding this comment

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

This piece is how we send plugin configuration (name, location, and soon type) to deck, so it can create tags for downloading these resources.

This PR uses init containers to download those resources, however if we don't utilize init containers, then we can just use the script tags to download resources where they're hosted (eg vs downloading them so Deck can host the resources).

Comment on lines +254 to +256
String localFilePath =
"/plugins/" + entry.getKey() + "/" + Paths.get(location).getFileName().toString();
resource.put("location", localFilePath);
Copy link
Member Author

Choose a reason for hiding this comment

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

this part will just be resource.put("location", location)
where location is the URL of the plugin resource

Profile pluginProfile =
pluginProfileFactory.getProfile(
pluginFilename, pluginPath, deploymentConfiguration, endpoints);
pluginProfileFactory.getProfileByName(
Copy link
Member Author

Choose a reason for hiding this comment

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

modified to support the new getProfileByName method, this should not need updating as we're going with the getProfile method

@@ -70,6 +75,100 @@ public boolean runsOnJvm() {
}
}

@Override
Copy link
Member Author

Choose a reason for hiding this comment

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

this is for creating the init containers to download the required resources. Since we're going to use script tags to download resources, we don't need to create init containers / volume mounts for this

@@ -72,7 +74,7 @@ protected void setProfile(
Profile profile,
DeploymentConfiguration deploymentConfiguration,
SpinnakerRuntimeSettings endpoints) {
StringResource configTemplate = new StringResource(profile.getBaseContents());
ObjectResource configTemplate = new ObjectResource(profile.getBaseContents());
Copy link
Member Author

Choose a reason for hiding this comment

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

profile.getBaseContents() works a bit different than the java services, in Deck's case, it downloads a settings.js template whose values are replaced via the StringResource. Since we do not control the settings.js template format, we'll probably have to do a string replace to add the plugins key and value, where the value should be the format given in ObjectReplaceTemplateResource

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.

1 participant