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

Move from variant map to array (rebased) #443

Merged
merged 5 commits into from
Jul 24, 2024

Conversation

dappnodedev
Copy link
Contributor

This PR overrides #440, as code base was out of date.

It aims to move from type:

interface BuildVariantsMap {
    [variant: string]: {
      // Compose file
      compose: Compose;
      
      // Manifest-related
      manifest: Manifest;
      manifestFormat: ManifestFormat;
      
      // File paths
      releaseDir: string;
      composePaths: string[];
      
      // Package information
      images: PackageImage[];
      architectures: Architecture[];
   }
}

to type:

interface PackageToBuildProps {
  variant: string | null;

  // Compose file
  compose: Compose;

  // Manifest-related
  manifest: Manifest;
  manifestFormat: ManifestFormat;

  // File paths
  releaseDir: string;
  composePaths: ComposePaths[];
  manifestPaths: ManifestPaths[];

  // Package information
  images: PackageImage[];
  architectures: Architecture[];
}[]

@dappnodedev dappnodedev requested a review from a team as a code owner July 15, 2024 15:17
Copy link

github-actions bot commented Jul 16, 2024

Dappnode bot has built and pinned the built packages to an IPFS node, for commit: 86edf17

This is a development version and should only be installed for testing purposes.

  1. Package dappnodesdk.public.dappnode.eth

Install link

Hash: /ipfs/QmcUPPYL5eUg2Cm6Wzkf7s8vzdYwm8tf3EWrqMKe98obVj

(by dappnodebot/build-action)

allVariants,
variantsStr,
rootDir,
variantsDirName,
variantsDirPath,
composeFileName
}: {
allVariants: boolean;
variantsStr?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isnt a bit confusing the name variantsStr? what about using variantsName ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the string you receive from console after flag --variants, like:
--variants mainnet,holesky,lukso

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated
// Manifest-related
manifest: Manifest;
manifestFormat: ManifestFormat;
}

export interface BuildVariantsMapEntry extends PublishVariantsMapEntry {
export interface PackageToBuildProps extends PackageToPublishProps {
variant: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your intentions allowing null if null means something either add a comment or find a way to define such intention with a boolean

Copy link
Contributor

Choose a reason for hiding this comment

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

also, what means exactly variant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment. variant could be mainnet, testnet, holesky... the value that makes one package differ from another in the same repo. It must be equal to the directory where that variant files are defined. For example, if you want to build the holesky and mainnet variants of a package, you would run:

`dappnodesdk build --variants-dir "./package_variants" --variants "mainnet,holesky"

And your repo must include these files:

.
├── avatar.png
├── dappnode_package.json
├── docker-compose.yml
...
├── package_variants
│   ├── holesky
│   │   ├── dappnode_package.json
│   │   └── docker-compose.yml
│   └── mainnet
│       ├── dappnode_package.json
│       └── docker-compose.yml
└── README.md

@@ -10,9 +10,9 @@ export interface CliGlobalOptions {

// TODO: Try to have all properties defined
interface ListrContextBuildItem {
releaseDir?: string;
variant: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as below. What means variant? what is the intention behind the scenes allowing null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Variant meaning is explained here: #443 (comment)

It can be null because legacy format packages (with full compose and manifest files in the root of the repo) have no variant so it makes sense that it is explicitly null

}: {
rootDir: string;
composeFileName: string;
variant: string;
variant: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

why allowing null? ist better the usage of a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I added a boolean, variant would still have to be undefined or null for single packages, so I would say it does not provide any value.

src/tasks/buildAndUpload/getUploadTasks.ts Outdated Show resolved Hide resolved
@dappnodedev dappnodedev merged commit acb2a0a into master Jul 24, 2024
8 of 10 checks passed
@dappnodedev dappnodedev deleted the feature/variants-map-to-array-v2 branch July 24, 2024 10:59
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.

2 participants