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

Use current active release to fork from #803

Closed
wants to merge 10 commits into from
Closed

Conversation

MiguelMoll
Copy link
Contributor

@MiguelMoll MiguelMoll commented Jun 23, 2016

Whenever ForkRelease is called it will now use the current release's build. Fixes #793

Also moved ReleaseLatest to the provider interface and added the Build ID to the output of convox releases.

Also in here, is a fix for convox run --release. If no releaseId is provided, convox run will use the last promoted release to run a process. Otherwise iterate over the previous revisions (starting with the
latest) looking for the releaseId specified. This makes sure the same environment variables are available. Closes #669

This does not resolve the issue where convox run can be used with a
release that hasn't been promoted yet. That will be tackled separately.

@@ -144,6 +144,11 @@ func (p *TestProviderRunner) ReleaseList(app string) (structs.Releases, error) {
return p.Releases, nil
}

func (p *TestProviderRunner) ReleaseLatest(app string) (*structs.Release, error) {

Choose a reason for hiding this comment

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

exported method TestProviderRunner.ReleaseLatest should have comment or be unexported

@nzoschke
Copy link
Contributor

The refactor looks great.

I'd like to start getting a bit more prescriptive about including tests with patches. I think provider interface work is the obvious place to start since the TestProvider is there to offer easier testing strategies.

Want to include some tests with this PR?

@MiguelMoll
Copy link
Contributor Author

Sure that sounds good to me. Also some output:

 convox releases
ID           CREATED             BUILD        STATUS
RLJCAORMDJT  3 minutes from now  BLOTHVWKJAE
RVLQLZVFOWF  3 minutes from now  BLOTHVWKJAE
RUZHEJXDDSV  10 minutes ago      BLOTHVWKJAE  active
RUYIJLIVPWA  10 minutes ago      BMMNUOMDCIE
RFQFSXCFKRF  11 minutes ago      BYCUTSQUQTQ

@MiguelMoll
Copy link
Contributor Author

Noah brought up:

In reviewing this I found a case to think about... Setting 2 environment variables, one at a time.
If we always fork from the current release, you can't progressively build up a new release.

@MiguelMoll MiguelMoll changed the title Use current release to fork from [WIP] Use current release to fork from Jun 23, 2016
@ddollar
Copy link
Contributor

ddollar commented Jun 23, 2016

I think the app's "current environment" is separate from the releases. We can pull the current environment from S3 rather than the latest or current release.

@@ -19,3 +19,11 @@ func NewRelease(app string) *Release {
Id: generateId("R", 10),
}
}

func (rs Releases) Latest() *Release {

Choose a reason for hiding this comment

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

exported method Releases.Latest should have comment or be unexported

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be susceptible to bugs in the future. There's no guarantee that this Releases struct is sorted appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Safer to check Created to get the latest one.

return names
}

func (e Environment) Raw() string {

Choose a reason for hiding this comment

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

exported method Environment.Raw should have comment or be unexported

@MiguelMoll
Copy link
Contributor Author

@MiguelMoll MiguelMoll changed the title [WIP] Use current release to fork from Use current active release to fork from Jun 27, 2016
release

If no releaseId is provided, `convox run` will use the last promoted release to run a
process. Otherwise iterate over the previous revisions (starting with the
latest) looking for the releaseId specified. This makes sure the same
environment variables are available.

This does not resolve the issue where `convox run` can be used with a
release that hasn't been promoted yet. That will be tackled separately.

Closes #669

ea := make([]string, 0)

Choose a reason for hiding this comment

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

can probably use "var ea []string" instead

@MiguelMoll
Copy link
Contributor Author

convox run --release

break
// If no releaseId is provided, use the last promoted release to run this process
// otherwise iterate over the previous revisions (starting with the latest) looking for the releaseId specified.
if releaseId == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good to me to retain the current run behavior with no release specified

@MiguelMoll MiguelMoll mentioned this pull request Jun 28, 2016
14 tasks
@MiguelMoll
Copy link
Contributor Author

Merged via #820

@MiguelMoll MiguelMoll deleted the fork-release-fix branch June 29, 2016 19:50
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