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

ci: update ci and add xtask testing #233

Merged
merged 1 commit into from
Feb 19, 2024
Merged

ci: update ci and add xtask testing #233

merged 1 commit into from
Feb 19, 2024

Conversation

gabriele-0201
Copy link
Contributor

No description provided.

Copy link
Contributor Author

gabriele-0201 commented Feb 15, 2024

@gabriele-0201 gabriele-0201 force-pushed the gab_add_xtask_on_ci branch 3 times, most recently from c45d353 to d974fca Compare February 15, 2024 14:16
@gabriele-0201 gabriele-0201 changed the base branch from main to gab_remove_node_in_path_requirement February 15, 2024 14:16
@gabriele-0201 gabriele-0201 force-pushed the gab_remove_node_in_path_requirement branch 2 times, most recently from 6a68add to 95b05a3 Compare February 15, 2024 14:51
@gabriele-0201 gabriele-0201 force-pushed the gab_remove_node_in_path_requirement branch from 95b05a3 to 03fead3 Compare February 16, 2024 12:33
@gabriele-0201 gabriele-0201 force-pushed the gab_remove_node_in_path_requirement branch from 03fead3 to 7bdb25b Compare February 16, 2024 16:15
@gabriele-0201 gabriele-0201 force-pushed the gab_add_xtask_on_ci branch 5 times, most recently from 4823336 to 6bc41b9 Compare February 18, 2024 09:16
@gabriele-0201 gabriele-0201 changed the base branch from gab_remove_node_in_path_requirement to gab_require_all_binaries_in_path February 18, 2024 09:16
@gabriele-0201 gabriele-0201 force-pushed the gab_require_all_binaries_in_path branch from 8b9f586 to 2486741 Compare February 18, 2024 09:20
@gabriele-0201 gabriele-0201 force-pushed the gab_require_all_binaries_in_path branch from 2486741 to 0d96325 Compare February 18, 2024 10:37
@gabriele-0201 gabriele-0201 force-pushed the gab_require_all_binaries_in_path branch from 0d96325 to 9af2377 Compare February 18, 2024 10:46
@gabriele-0201 gabriele-0201 marked this pull request as ready for review February 18, 2024 10:48
docker/gha-runner.Dockerfile Outdated Show resolved Hide resolved
RUN apt-get install -y nodejs multitail
RUN npm install -g @zombienet/cli

COPY --from=parity/polkadot:v1.6.0 /usr/bin/polkadot /usr/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point we could also extract this into an ARG.

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 discovered that using ARG is more complex than expected. Doing something like this:

ARG POLKADOT=parity/polkadot:v1.6.0
FROM $POLKADOT as polkadot
COPY --from=polkadot /usr/bin/polkadot /usr/bin/

does not work because ARG can be defined at build time with an argument variable, and parity/polkadot:v1.6.0 would be used only if it is not specified. However, if it is specified but not assigned to anything, it would be empty, which is not valid for the FROM command. To make it work, it should be:

ARG POLKADOT=parity/polkadot:v1.6.0
FROM ${POLKADOT:-parity/polkadot:v1.6.0} as polkadot
COPY --from=polkadot /usr/bin/polkadot /usr/bin/

But if the goal is to reduce the lines that need to be changed on a Polkadot update, I opted for:

FROM parity/polkadot:v1.6.0 as polkadot
COPY --from=polkadot /usr/bin/polkadot /usr/bin/

xtask/src/main.rs Outdated Show resolved Hide resolved
xtask/src/main.rs Outdated Show resolved Hide resolved
Base automatically changed from gab_require_all_binaries_in_path to main February 19, 2024 09:15

std::env::set_var(
"CONSTANTS_MANIFEST",
format!("{}demo/sovereign/constants.json", project_dir),
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems finicky: seems like you are assuming that project_dir will always come with a trailing slash, will that always be the case? Won't it be better to use Path? As a bonus you will be able to check the directory existence.

Copy link
Contributor Author

@gabriele-0201 gabriele-0201 Feb 19, 2024

Choose a reason for hiding this comment

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

In the way I created the regex, it will contain the \, but you pointed out something really interesting. By using Path, the code becomes much cleaner and less errors prone

        let path = std::path::Path::new(project_dir).join("demo/sovereign/constants.json");
        if !path.exists() {
            bail!(
                "The `constants.json` file for Sovereign does not exist,\n \
                   or it is not in the expected position, `demo/sovereign/constants.json`"
            )
        }
        std::env::set_var("CONSTANTS_MANIFEST", path);

@pepyakin pepyakin force-pushed the gab_add_xtask_on_ci branch from 9c6168c to 03ddb09 Compare February 19, 2024 11:12
@pepyakin pepyakin merged commit 13c3a5a into main Feb 19, 2024
6 checks passed
@pepyakin pepyakin deleted the gab_add_xtask_on_ci branch February 19, 2024 13:38
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