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

Refactor vacation.Executable #260

Merged
merged 2 commits into from
Dec 6, 2021
Merged

Conversation

ForestEckhardt
Copy link
Contributor

  • NopArchive will now no longer write directly to the destination if a
    name is not given because it now set a default name (artifact). If you
    were using the generic Archive type you will see no difference but those
    who were using the type directly must now specify a directory as the
    destination, not a file name.
  • NewExecutable() now conforms to the same api as the other archive
    instantiation functions excepting only a reader and allowing individuals
    to set the file name using a WithName() option with mirrors the
    behavior of all other archive objects.
    -Executable now has a default file name (artifact) if you were using
    the generic Archive type you will see no difference however because the
    Executable instantiation function no longer requires a name a default is
    now assigned on instantiation the same way it is done for the
    NopArchive.
  • Executable no longer places the executable in a directory name bin
    nor does it create all directories in the destination path. I felt that
    automatically placing the executable in a bin directory felt to
    opinionated, I think that it is more than reasonable for a user to want
    to download and executable and not want to have it placed in a directory
    named bin and this default behavior hinders that user. I think that it
    is more than reasonable to expect that the destination given is a bin
    directory if that is the desired effect. As for no longer creating all
    of directories in the destination path, NopArchive is precedent setting
    as it expects to write files only to destinations that exist already.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

- Updates documentation examples to demonstrate that compressed files
are now handled instead of compressed tar archives
- NopArchive will now no longer write directly to the destination if a
name is not given becuase it now set a default name (`artifact`). If you
were using the generic Archive type you will see no difference but those
who were using the type directly must now specify a directory as the
destination, not a file name.
- NewExecutable now conforms to the same api as the other archive
instantiation functions excepting only a reader and allowing individuals
to set the file name using a `WithName()` option with mirrors the
behavior of all other archive objects.
-Executable now has a default file name (`artifact`) if you were using
the generic Archive type you will see no difference however becuase the
Executable instantiation function no longer requires a name a default is
now assigned on instantiation the same way it is done for the
NopArchive.
- Excutable no longer places the executable in a directory name `bin`
nor does it create all directories in the destination path. I felt that
automatically placing the executable in a `bin` directory felt to
opinionated, I think that it is more than reasonable for a user to want
to download and executable and not want to have it placed in a directory
named `bin` and this default behavior hinders that user. I think that it
is more than reasonable to expect that the destination given is a `bin`
directory if that is the desired effect. As for no longer creating all
of directories in the destination path, NopArchive is precedent setting
as it expects to write files only to destinations that exist already.
@ForestEckhardt ForestEckhardt requested a review from a team as a code owner December 2, 2021 23:09
@ForestEckhardt
Copy link
Contributor Author

@mboldt I have made a couple of changes to you initial API in this PR. I would love if you were to take a look and give me any feedback/disagreements that you might have with my changes. I am more than open for conversation on this one bearing in mind that the generic Archive api for executables have not changed at all just the underlying Executable api meaning that it should behave identically if you are using something like postal.Service.Deliver().

@ForestEckhardt ForestEckhardt added the semver:major A change requiring a major version bump label Dec 2, 2021
@ForestEckhardt ForestEckhardt added this to the v2.0.0 milestone Dec 2, 2021
@mboldt
Copy link
Contributor

mboldt commented Dec 3, 2021

All makes sense.

I did the bin/ dir thing since it ends up on the PATH for buildpacks, which I expect is the common case. But, I agree with your argument that it breaks precedent and is presumptuous! Perhaps renaming the postal.Service.Deliver() arg from layerPath to destinationDirectory or somesuch would make it clear that one can pass in the bin/ directory instead of the root layer path.

Thank you for checking in, and appreciate the cleanup!

@ryanmoran ryanmoran merged commit 9e15703 into main Dec 6, 2021
@ryanmoran ryanmoran deleted the refactor-vacation-executable branch December 6, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major A change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants