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

[APM] Use apm_8.0.0 archive where possible #77450

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

dgieselaar
Copy link
Member

No description provided.

@dgieselaar dgieselaar added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 15, 2020
@dgieselaar dgieselaar requested a review from a team September 15, 2020 09:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@@ -10,6 +10,8 @@ export default function ApiTest({ getService }: FtrProviderContext) {
const supertest = getService('supertest');
const esArchiver = getService('esArchiver');

const archiveName = 'apm_8.0.0';
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned I think we should pin this to a specific version.
The current change is a good example: archive 8.0.0 is being replaced by archive 8.0.0. It's impossible to see what changed. At a minimum we should have a date that accompanies the archive if we choose a non-released, floating version.

Copy link
Member

@sorenlouv sorenlouv Sep 15, 2020

Choose a reason for hiding this comment

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

Btw. why is the archive called apm_8.0.0? Is the apm prefix needed to distinguish from other solutions' archives?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually not an archive update, but simply using our main archive. Fwiw, if we have a more specific version in our name, it will be harder for us to automate an archive update.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need it, I think my intention was to distinguish from other archives like the observability one.

Copy link
Member

@sorenlouv sorenlouv Sep 15, 2020

Choose a reason for hiding this comment

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

okay i see it's not an archive update (they both already exist) but how do we distinguish between the two?

I assume we want to delete the old one?

Fwiw, if we have a more specific version in our name, it will be harder for us to automate an archive update.

Would this also be the case if we prefix it with a date?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sqren I would like to delete 8.0.0, but it's used in conjunction with rum_8.0.0, and I don't know what's in the latter. (I could find out, but for this PR I want to improve coverage of the main/apm_8.0.0 archive so I can upgrade it in a second PR, in preparation for the Metric Powered KPIs tests). Would you be okay with me opening up an issue around archive naming? I can pick it up after I merge Metric Powered KPIs.

Copy link
Member

Choose a reason for hiding this comment

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

Sure feel free to merge. I don't think we need an issue for this. But having two almost identically named archives is going to cause confusion so we should fix it.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants