Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

go/tools/scion-pki/internal/v2/conf/trc.go, line 111 at r1 (raw file):

type Primary struct {
	Attributes              []trc.Attribute     `toml:"attributes"`
	IssuingKeyVersion       *scrypto.KeyVersion `toml:"issuing_key_version"`

Why is this a pointer instead of a value? Every read risks causing a panic this way.

If it's just to support default values because 0 is a special cased value, then it's ok provided this never gets seen out of internal.

I assume the same reasoning applies to the fields below.

go/tools/scion-pki/internal/v2/conf/trc.go, line 117 at r1 (raw file):

// Validate checks the right keys are set.
func (p Primary) Validate() error {

This will return nil for something like:

func TestXxx(t *testing.T) {
	p := conf.Primary{
		Attributes: []trc.Attribute{"foo"},
	}
	assert.Error(t, p.Validate())
}

So it looks like "deep validation" (validation of all fields) isn't provided by the API. If this is intended, is should be explicitly mentioned in the godoc.

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @scrye)

go/tools/scion-pki/internal/v2/conf/trc.go, line 111 at r1 (raw file):

Well somewhat.

0 is not special cased, but a valid value.
It has to be a pointer, because that is the only way of distinguishing whether the value was actually set in the toml file.

I could implement the toml.Unmarshaler interface, but that is overkill IMO.

go/tools/scion-pki/internal/v2/conf/trc.go, line 117 at r1 (raw file):

That should never be the case in normal operation because trc.Attribute implements UnmarshalText that has the validity check, and attributes always stem from parsing, or constant value.

Added for completeness.

Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

go/tools/scion-pki/internal/v2/conf/trc.go, line 117 at r1 (raw file):

That assumes only UnmarshalText is used to construct objects of type trc.Attribute. But given the exported type and fields that may not always be the case.

It's better to be paranoid, and assume the users of your API will use it in unintended ways (often due to a lack of familiarity). And once it's used the wrong way and it works, it might be the case that it can no longer be changed.

Reviewable status: complete! all files reviewed, all discussions resolved