diff --git a/pkg/cmd/package/nuget/create/create.go b/pkg/cmd/package/nuget/create/create.go
index 72d7c66f..bc529fee 100644
--- a/pkg/cmd/package/nuget/create/create.go
+++ b/pkg/cmd/package/nuget/create/create.go
@@ -1,9 +1,12 @@
package create
import (
+ "bytes"
+ "encoding/xml"
"errors"
"fmt"
"os"
+ "os/user"
"path/filepath"
"strings"
"time"
@@ -27,6 +30,11 @@ const (
FlagReleaseNotesFile = "releaseNotesFile"
)
+// DefaultDescription is used when the caller supplies no description. The nuspec
+// schema requires the element, and this is what the flag help has always
+// promised.
+const DefaultDescription = "A deployment package created from files on disk."
+
type NuPkgCreateFlags struct {
Author *flag.Flag[[]string] // this need to be multiple and default to current user
Title *flag.Flag[string]
@@ -112,23 +120,33 @@ func createRun(cmd *cobra.Command, opts *NuPkgCreateOptions) error {
return err
}
- nuspecFilePath := ""
- if shouldGenerateNuSpec(opts) {
- defer func() {
- if nuspecFilePath != "" {
- err := os.Remove(nuspecFilePath)
- if err != nil {
- panic(err)
- }
- }
- }()
- nuspecFilePath, err = GenerateNuSpec(opts)
+ // Every .nupkg needs a manifest: readers locate it by looking for a single
+ // .nuspec in the archive root, so a package without one is not a valid NuGet
+ // package even though it is a perfectly good zip. Generate one unless the
+ // base path already supplies it, in which case that file is authoritative and
+ // must not be touched.
+ nuspecFileName := opts.Id.Value + ".nuspec"
+ suppliedNuSpec, err := hasSuppliedNuSpec(opts.BasePath.Value, nuspecFileName)
+ if err != nil {
+ return err
+ }
+
+ if suppliedNuSpec {
+ pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Using existing nuspec file \"%s\"\n", nuspecFileName)
+ } else {
+ nuspecFilePath, err := GenerateNuSpec(opts)
if err != nil {
return err
}
- opts.Include.Value = append(opts.Include.Value, opts.Id.Value+".nuspec")
+ defer func() {
+ if err := os.Remove(nuspecFilePath); err != nil {
+ pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Could not remove generated nuspec file \"%s\": %v\n", nuspecFilePath, err)
+ }
+ }()
}
+ opts.Include.Value = append(opts.Include.Value, nuspecFileName)
+
pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Packing \"%s\" version \"%s\"...\n", opts.Id.Value, opts.Version.Value)
outFilePath := pack.BuildOutFileName("nupkg", opts.Id.Value, opts.Version.Value)
@@ -198,7 +216,7 @@ func PromptMissing(opts *NuPkgCreateOptions) error {
if err := opts.Ask(&survey.Input{
Message: "Nuspec description",
Help: "The description to include in the Nuspec file.",
- Default: "A deployment package created from files on disk.",
+ Default: DefaultDescription,
}, &opts.Description.Value); err != nil {
return err
}
@@ -246,15 +264,38 @@ func applyDefaultsToUnspecifiedPackageOptions(opts *NuPkgCreateOptions) error {
opts.Include.Value = append(opts.Include.Value, "**")
}
- if len(opts.Author.Value) > 0 {
- if opts.Description.Value == "" {
- opts.Description.Value = "A deployment package created from files on disk."
- }
+ // A manifest is generated for every package now, so these two are no longer
+ // only relevant when the caller opted into metadata: the nuspec schema
+ // requires both, and leaving them out produces a package strict readers
+ // reject.
+ if opts.Description.Value == "" {
+ opts.Description.Value = DefaultDescription
+ }
+
+ if util.Empty(opts.Author.Value) {
+ opts.Author.Value = []string{defaultAuthor(opts.Id.Value)}
}
return nil
}
+// defaultAuthor mirrors what the old Octopus CLI did, and what the Author flag
+// has always said it should do. Where the user cannot be determined the package
+// ID stands in: the element is required, and an obvious placeholder beats
+// failing the command over metadata nobody asked for.
+// currentUser is a seam: user lookup fails on some minimal container images, and
+// the fallback needs to be reachable in a test.
+var currentUser = user.Current
+
+func defaultAuthor(fallback string) string {
+ if current, err := currentUser(); err == nil {
+ if name := strings.TrimSpace(current.Username); name != "" {
+ return name
+ }
+ }
+ return fallback
+}
+
func getReleaseNotesFromFile(filePath string) (string, error) {
_, err := os.Stat(filePath)
if err != nil {
@@ -269,22 +310,34 @@ func getReleaseNotesFromFile(filePath string) (string, error) {
return string(notes), nil
}
-func shouldGenerateNuSpec(opts *NuPkgCreateOptions) bool {
- return opts.Description.Value != "" ||
- opts.Title.Value != "" ||
- opts.ReleaseNotes.Value != "" ||
- opts.ReleaseNotesFile.Value != "" ||
- !util.Empty(opts.Author.Value)
+// escapeXML makes a value safe to drop between two tags. Release notes and
+// descriptions are free text, and an unescaped ampersand or angle bracket is
+// enough to make the whole manifest unparseable.
+func escapeXML(value string) string {
+ var buf bytes.Buffer
+ if err := xml.EscapeText(&buf, []byte(value)); err != nil {
+ // EscapeText only fails if the writer fails, and bytes.Buffer does not.
+ return value
+ }
+ return buf.String()
}
-func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) {
-
- if opts.Description.Value == "" {
- return "", errors.New("description is required when generating nuspec metadata")
+// hasSuppliedNuSpec reports whether the base path already contains a manifest
+// for this package. One written by hand is the user's own file: it is theirs to
+// keep, and generating over the top of it would both discard their metadata and
+// delete the file on the way out.
+func hasSuppliedNuSpec(basePath string, nuspecFileName string) (bool, error) {
+ _, err := os.Stat(filepath.Join(basePath, nuspecFileName))
+ if err == nil {
+ return true, nil
}
- if len(opts.Author.Value) == 0 {
- return "", errors.New("at least one author is required when generating nuspec metadata")
+ if os.IsNotExist(err) {
+ return false, nil
}
+ return false, err
+}
+
+func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) {
releaseNotes := opts.ReleaseNotes.Value
if opts.ReleaseNotesFile.Value != "" {
@@ -305,15 +358,15 @@ func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) {
sb.WriteString(`` + "\n")
sb.WriteString(`` + "\n")
sb.WriteString(" \n")
- sb.WriteString(" " + opts.Id.Value + "\n")
- sb.WriteString(" " + opts.Version.Value + "\n")
+ sb.WriteString(" " + escapeXML(opts.Id.Value) + "\n")
+ sb.WriteString(" " + escapeXML(opts.Version.Value) + "\n")
if opts.Title.Value != "" {
- sb.WriteString(" " + opts.Title.Value + "\n")
+ sb.WriteString(" " + escapeXML(opts.Title.Value) + "\n")
}
- sb.WriteString(" " + opts.Description.Value + "\n")
- sb.WriteString(" " + strings.Join(opts.Author.Value, ",") + "\n")
+ sb.WriteString(" " + escapeXML(opts.Description.Value) + "\n")
+ sb.WriteString(" " + escapeXML(strings.Join(opts.Author.Value, ",")) + "\n")
if releaseNotes != "" {
- sb.WriteString(" " + releaseNotes + "\n")
+ sb.WriteString(" " + escapeXML(releaseNotes) + "\n")
}
sb.WriteString(" \n")
sb.WriteString("\n")
diff --git a/pkg/cmd/package/nuget/create/create_test.go b/pkg/cmd/package/nuget/create/create_test.go
new file mode 100644
index 00000000..d535a5f0
--- /dev/null
+++ b/pkg/cmd/package/nuget/create/create_test.go
@@ -0,0 +1,221 @@
+package create
+
+import (
+ "encoding/xml"
+ "errors"
+ "os"
+ "os/user"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ pack "github.com/OctopusDeploy/cli/pkg/cmd/package/support"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// nuspec is the subset of the manifest these tests care about, used to assert
+// against parsed XML rather than raw substrings.
+type nuspec struct {
+ Metadata struct {
+ Id string `xml:"id"`
+ Version string `xml:"version"`
+ Title string `xml:"title"`
+ Description string `xml:"description"`
+ Authors string `xml:"authors"`
+ ReleaseNotes string `xml:"releaseNotes"`
+ } `xml:"metadata"`
+}
+
+func newTestOptions(t *testing.T, basePath string) *NuPkgCreateOptions {
+ t.Helper()
+
+ opts := &NuPkgCreateOptions{
+ NuPkgCreateFlags: NewNuPkgCreateFlags(),
+ PackageCreateOptions: &pack.PackageCreateOptions{PackageCreateFlags: pack.NewPackageCreateFlags()},
+ }
+ opts.Id.Value = "Acme.Web"
+ opts.Version.Value = "1.2.3"
+ opts.BasePath.Value = basePath
+ return opts
+}
+
+func generateAndParse(t *testing.T, opts *NuPkgCreateOptions) (nuspec, string) {
+ t.Helper()
+
+ path, err := GenerateNuSpec(opts)
+ require.NoError(t, err)
+
+ contents, err := os.ReadFile(path)
+ require.NoError(t, err)
+
+ var parsed nuspec
+ require.NoError(t, xml.Unmarshal(contents, &parsed), "generated nuspec should be well-formed XML")
+
+ return parsed, string(contents)
+}
+
+// The manifest is what makes a .nupkg a NuGet package rather than a zip, and
+// the OPC relationships part points at it by name whether or not it exists.
+func TestGeneratesNuSpecWithNoMetadataSupplied(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ parsed, _ := generateAndParse(t, opts)
+
+ assert.Equal(t, "Acme.Web", parsed.Metadata.Id)
+ assert.Equal(t, "1.2.3", parsed.Metadata.Version)
+ assert.NotEmpty(t, parsed.Metadata.Description, "description is required by the nuspec schema")
+ assert.NotEmpty(t, parsed.Metadata.Authors, "authors is required by the nuspec schema")
+}
+
+func TestDescriptionDefaultsWhenNotSupplied(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ assert.Equal(t, DefaultDescription, opts.Description.Value)
+}
+
+func TestSuppliedDescriptionIsKept(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ opts.Description.Value = "Something specific"
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ assert.Equal(t, "Something specific", opts.Description.Value)
+}
+
+func TestAuthorDefaultsToCurrentUser(t *testing.T) {
+ current, err := user.Current()
+ require.NoError(t, err)
+
+ opts := newTestOptions(t, t.TempDir())
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ assert.Equal(t, []string{current.Username}, opts.Author.Value)
+}
+
+func TestSuppliedAuthorsAreKept(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ opts.Author.Value = []string{"Ada", "Grace"}
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ parsed, _ := generateAndParse(t, opts)
+
+ assert.Equal(t, []string{"Ada", "Grace"}, opts.Author.Value)
+ assert.Equal(t, "Ada,Grace", parsed.Metadata.Authors)
+}
+
+// User lookup fails on some minimal container images. The command should still
+// produce a schema-valid manifest rather than failing over metadata nobody asked
+// for.
+func TestDefaultAuthorFallsBackWhenUserIsUnknown(t *testing.T) {
+ original := currentUser
+ t.Cleanup(func() { currentUser = original })
+ currentUser = func() (*user.User, error) { return nil, errors.New("no user") }
+
+ assert.Equal(t, "Acme.Web", defaultAuthor("Acme.Web"))
+}
+
+func TestDefaultAuthorFallsBackWhenUsernameIsBlank(t *testing.T) {
+ original := currentUser
+ t.Cleanup(func() { currentUser = original })
+ currentUser = func() (*user.User, error) { return &user.User{Username: " "}, nil }
+
+ assert.Equal(t, "Acme.Web", defaultAuthor("Acme.Web"))
+}
+
+func TestPackageIsStillUsableWhenUserLookupFails(t *testing.T) {
+ original := currentUser
+ t.Cleanup(func() { currentUser = original })
+ currentUser = func() (*user.User, error) { return nil, errors.New("no user") }
+
+ opts := newTestOptions(t, t.TempDir())
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ parsed, _ := generateAndParse(t, opts)
+
+ assert.Equal(t, "Acme.Web", parsed.Metadata.Authors)
+}
+
+// A hand-written manifest is the user's own file. Generating over the top would
+// discard their metadata, and the cleanup step would then delete it outright.
+func TestExistingNuSpecIsDetected(t *testing.T) {
+ basePath := t.TempDir()
+ existing := filepath.Join(basePath, "Acme.Web.nuspec")
+ require.NoError(t, os.WriteFile(existing, []byte("Acme.Web"), 0644))
+
+ supplied, err := hasSuppliedNuSpec(basePath, "Acme.Web.nuspec")
+
+ require.NoError(t, err)
+ assert.True(t, supplied)
+}
+
+func TestMissingNuSpecIsNotMistakenForOne(t *testing.T) {
+ supplied, err := hasSuppliedNuSpec(t.TempDir(), "Acme.Web.nuspec")
+
+ require.NoError(t, err)
+ assert.False(t, supplied)
+}
+
+func TestTitleIsOmittedWhenNotSupplied(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ _, raw := generateAndParse(t, opts)
+
+ assert.NotContains(t, raw, "")
+}
+
+func TestReleaseNotesAreIncludedWhenSupplied(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ opts.ReleaseNotes.Value = "Fixed a thing"
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ parsed, _ := generateAndParse(t, opts)
+
+ assert.Equal(t, "Fixed a thing", parsed.Metadata.ReleaseNotes)
+}
+
+func TestReleaseNotesAndReleaseNotesFileAreMutuallyExclusive(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ opts.ReleaseNotes.Value = "Fixed a thing"
+ opts.ReleaseNotesFile.Value = "notes.txt"
+
+ _, err := GenerateNuSpec(opts)
+
+ assert.ErrorContains(t, err, "cannot specify both")
+}
+
+// Descriptions and release notes are free text. An unescaped ampersand or angle
+// bracket is enough to make the whole manifest unparseable, which matters more
+// now that one is written for every package.
+func TestMetadataIsXmlEscaped(t *testing.T) {
+ opts := newTestOptions(t, t.TempDir())
+ opts.Description.Value = `Fish & chips <b>bold</b>`
+ opts.Title.Value = `A "quoted" title`
+ opts.ReleaseNotes.Value = `1 < 2 && 3 > 2`
+ opts.Author.Value = []string{"Ada & Grace"}
+
+ parsed, raw := generateAndParse(t, opts)
+
+ assert.NotContains(t, raw, "Fish & chips", "raw ampersand should have been escaped")
+ assert.Contains(t, raw, "&")
+
+ // Round-tripping is the real assertion: what went in is what comes back out.
+ assert.Equal(t, `Fish & chips <b>bold</b>`, parsed.Metadata.Description)
+ assert.Equal(t, `A "quoted" title`, parsed.Metadata.Title)
+ assert.Equal(t, `1 < 2 && 3 > 2`, parsed.Metadata.ReleaseNotes)
+ assert.Equal(t, "Ada & Grace", parsed.Metadata.Authors)
+}
+
+func TestNuSpecIsWrittenIntoTheBasePath(t *testing.T) {
+ basePath := t.TempDir()
+ opts := newTestOptions(t, basePath)
+ require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts))
+
+ path, err := GenerateNuSpec(opts)
+
+ require.NoError(t, err)
+ assert.Equal(t, filepath.Join(basePath, "Acme.Web.nuspec"), path)
+ assert.True(t, strings.HasSuffix(path, ".nuspec"))
+}