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.

Sign up for GitHub

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

Jump to bottom

imp(ci): remove duplicated test-fake-activation-heights test #4118

Closed
wants to merge 13 commits into from
Closed

imp(ci): remove duplicated test-fake-activation-heights test #4118

wants to merge 13 commits into from

Conversation

gustavovalverde
Copy link
Member

@gustavovalverde gustavovalverde commented Apr 14, 2022 •
edited
Loading

Motivation

test-fake-activation-heights should be removed from:

zebra/.github/workflows/os-build-tests.yml

Line 128 in 9e0bbb5

test-fake-activation-heights:
as we already have it here

zebra/.github/workflows/continous-integration.yml

Line 161 in 9e0bbb5

test-fake-activation-heights:

Depends-On: #3941

Solution

  • Remove it from the test-os-builds.yml

Review

Anyone can review this

@gustavovalverde gustavovalverde added A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-Low ❄️ labels Apr 14, 2022
@gustavovalverde gustavovalverde requested a review from a team as a code owner April 14, 2022 02:12
@gustavovalverde gustavovalverde self-assigned this Apr 14, 2022
@gustavovalverde gustavovalverde requested review from conradoplg and removed request for a team April 14, 2022 02:12
@gustavovalverde gustavovalverde changed the title Remove extra test imp(ci): remove duplicated test-fake-activation-heights test Apr 14, 2022
@gustavovalverde gustavovalverde changed the title imp(ci): remove duplicated test-fake-activation-heights test imp(ci): remove duplicated test-fake-activation-heights test Apr 14, 2022
@gustavovalverde gustavovalverde changed the base branch from main to imp-workflow-names April 14, 2022 02:20
conradoplg
conradoplg requested changes Apr 19, 2022
View reviewed changes
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

I feel like it should be the other way around: leave it in os-build and remove from continous-integration, since it's just a unit test. Was there any particular reason for doing in this way?

teor2345
teor2345 reviewed Apr 19, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Did you also want to remove other redundant jobs in this PR?

Some steps in the Build stable on ubuntu-latest job are redundant, because they are covered by:

  • Install zebrad from lockfile without cache on ubuntu-latest
  • Test stable on ubuntu-latest

We still need to Check Cargo.lock is up to date, but we can remove the steps after that. (And rename the job!)

@gustavovalverde gustavovalverde marked this pull request as draft April 22, 2022 11:24
@teor2345
Copy link
Contributor

Please include the clean fix #4206 when next updating this branch.

Base automatically changed from imp-workflow-names to main May 9, 2022 19:54
@gustavovalverde
Copy link
Member Author

Closed as further decisions must be taken around this workflow

@gustavovalverde gustavovalverde deleted the remove-extra-test branch May 10, 2022 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 left review comments

@conradoplg conradoplg conradoplg requested changes

Assignees

@gustavovalverde gustavovalverde

Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gustavovalverde @teor2345 @conradoplg