-
Notifications
You must be signed in to change notification settings - Fork 702
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
Set a deployment's version with Halyard #201
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -32,9 +34,16 @@ | |||
public class ConfigProblemSetBuilder { | |||
private List<ConfigProblemBuilder> builders = new ArrayList<>(); | |||
|
|||
@Getter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when you leave off the AccessLevel
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It defaults to public. Everything in the core/config code has public getters & setters, it's only the CLI package where I need to be careful to delineate what to generate.
@@ -12,9 +12,10 @@ | |||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
* See the License for the specific language governing permissions and | |||
* limitations under the License. | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely Intellij during the automatic refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Parameter( | ||
names = "--version", | ||
required = true, | ||
description = "Must be either a version number \"X.Y.Z\" for a specific release of Spinnaker, \"latest\" for the latest " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop for the latest
at the end of the line here.
|
||
protected List<String> versionOptions(ConfigProblemSetBuilder psBuilder) { | ||
VersionsService service = psBuilder.getContext().getBean(VersionsService.class); | ||
return service.getVersions().getVersions().stream().map(Version::getVersion).collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getVersions().getVersions()
is a little tough to read. Could the first be loadVersions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but then you'd have two methods, one returning a Versions
object via get
, and another returning a list of Versions.Version
objects via load
, which is confusing.
This turned into more work than expected because
halyard-core
.ProblemSets
with the application context to pass to any validation fixed this. This lets us recommend versions like so: