feat: implement new convert command

[hbagdi]

No description provided.

[mflendrich]

This review is from a standpoint of concern that the quality of deck's codebase may be at risk of many small changes introducing small bits of tech debt and, as a reviewer, I'd like to deal with that by:

• preventing (or at least limiting) code's side effects (for example mutating arguments),
• enforcing that new code follows SOLID (most notably the "S") where practical

Several (non-binding non-blocking) style preferences attached as well.

Test coverage not reviewed - that will happen in another round.

[codecov-commenter]

Codecov Report

Merging #330 (8a31fee) into main (7c3660c) will increase coverage by 0.42%.
The diff coverage is 77.64%.

❗ Current head 8a31fee differs from pull request most recent head 02d28b7. Consider uploading reports for the commit 02d28b7 to get more accurate results

@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
+ Coverage   53.57%   53.99%   +0.42%     
==========================================
  Files          59       61       +2     
  Lines        4885     4967      +82     
==========================================
+ Hits         2617     2682      +65     
- Misses       1970     1986      +16     
- Partials      298      299       +1     

Impacted Files	Coverage Δ
file/writer.go	15.28% <33.33%> (ø)
cmd/convert.go	37.50% <37.50%> (ø)
convert/convert.go	96.55% <96.55%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c3660c...02d28b7. Read the comment docs.

[hbagdi]

@mflendrich I rebased this on top of #332 and ensured that input arguments are not being modified. PTAL.

[hbagdi]

@mflendrich Friendly ping for another review.

[mflendrich]

Comment part 1:

This duplicates the logic of the switch in func Convert(). Why do we need both? Why not just let Convert() fail if no suitable conversion is possible?

Comment part 2 (provided that we need to retain this function for some reason):

Each of the two return values of this function gives exact same information - the error additionally giving the error message. This makes the return value space unnecessarily complex: ((true, nil), (true, err), (false, nil), (false, err)).

If you rephrase this function as func validateConversion(from, to Format) error, you'll have only one return value. Easier to test and understand, more difficult to use in a buggy way.

Why: imagine that this function somehow ends up returning false, nil (because this is a natural way to say "the result evaluated to false with no runtime errors". In such case, the caller (func Convert) will erroneously return nil (saying "no error").

[mflendrich]

We're passing inputFilename and outputFilename down the function calls several times without a clear reason, causing convertKongGatewayToKonnect to be concerned not only with conversion logic, but also with file handling.

Also, with more conversion format pairs added, file handling logic will need to be copied across all these format pair implementations.

How about doing file operations in Convert and having conversions as simple functional transformations (without the need to bloat actual conversion logic with the file handling concern)

func Convert(inputFilename, outputFilename string, from, to Format) error {
	// probably unnecessary - see my comment above
	if valid, err := validConversion(from, to); !valid {
		return err
	}

	inputContent, err := file.GetContentFromFiles([]string{inputFilename})
	if err != nil {
		return err
	}

	var outputContent *file.Content
	switch {
	case from == FormatKongGateway && to == FormatKonnect:
		outputContent = convertKongGatewayToKonnect(inputContent)
	default:
		return fmt.Errorf("cannot convert from '%s' to '%s' format", from, to)
	}

	return file.WriteContentToFile(outputContent, outputFilename, file.YAML)
}

[mflendrich]

func wipeServiceName(s *file.Service) *file.Service {
  result := s.DeepCopy()
  result.Name = nil
  result.ID = kong.String(utils.UUID())
  return result
}

and

Suggested change

	Service: serviceCopy,
	Service: wipeServiceName(service),

[hbagdi]

@mflendrich Resolved the comments. PTAL.

[hbagdi]

ping for another review @mflendrich