Build zipped package by mtojek · Pull Request #550 · elastic/elastic-package

mtojek · 2021-10-21T16:19:14Z

Issue: elastic/package-registry#728

This PR modifies:
-build command to also built .zip packages (--zip flag)
-clean command to clean built .zip

elasticmachine · 2021-10-21T16:24:21Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-10-25T13:29:46.535+0000
Duration: 35 min 45 sec
Commit: 92ef5d7

Test stats 🧪

Test	Results
Failed	0
Passed	438
Skipped	4
Total	442

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

mtojek · 2021-10-21T16:57:38Z

The package registry tried to load the .zip archive, but it failed:

�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zookeeper           	     0.4.0	/packages/snapshot/zookeeper/0.4.0
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zookeeper           	     1.0.0	/packages/snapshot/zookeeper/1.0.0
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zscaler             	     0.3.0	/packages/snapshot/zscaler/0.3.0
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zscaler             	     0.3.1	/packages/snapshot/zscaler/0.3.1
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zscaler             	     0.3.2	/packages/snapshot/zscaler/0.3.2
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zscaler             	     0.3.3	/packages/snapshot/zscaler/0.3.3
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zscaler             	     0.4.1	/packages/snapshot/zscaler/0.4.1
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zscaler             	     0.4.2	/packages/snapshot/zscaler/0.4.2
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 zscaler             	     0.4.3	/packages/snapshot/zscaler/0.4.3
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 Packages in /packages/development:
�[36mpackage-registry_1           |�[0m 2021/10/21 16:54:51 reading packages from filesystem failed: loading package failed (path: /packages/development/apache-0.0.1.zip): failed to determine root directory in package (path: /packages/development/apache-0.0.1.zip)

@jsoriano should I start the EPR differently?

jsoriano · 2021-10-25T09:39:11Z

@jsoriano should I start the EPR differently?

The problem seems to be in the format of the package. Implementation in package-registry expects to have the files under the <package>-<version> directory, but this implementation is archiving the files directly in the root directory:

This is an example package in the package registry repo:

$ unzip -l testdata/local-storage/example-1.0.1.zip 
Archive:  testdata/local-storage/example-1.0.1.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  2021-07-29 19:26   example-1.0.1/
      516  2021-07-29 19:26   example-1.0.1/manifest.yml
...

This is a package built with this implementation:

$ unzip -l build/integrations/apache-0.0.1.zip
Archive:  build/integrations/apache-0.0.1.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
      234  2021-10-25 11:27   changelog.yml
        0  2021-10-25 11:27   data_stream/
        0  2021-10-25 11:27   data_stream/access/
        0  2021-10-25 11:27   data_stream/access/agent/
...

I think I did this because I don't like archives that you decompress and they spread all over the current directory 🙂 But we are probably in time to reconsider this format if needed.

(Btw, side thoughts now, having the package assets inside a directory might allow to embed in the zip other validation or control data, like a signature 🙂)

mtojek · 2021-10-25T09:47:00Z

I think we need to be consistent with current .zip files, for example: https://epr.elastic.co/epr/aws/aws-1.1.0.zip , so I will adjust the logic here :)

(Btw, side thoughts now, having the package assets inside a directory might allow to embed in the zip other validation or control data, like a signature 🙂)

So with this approach we agree on the content addressed signatures. WDYT?

mtojek · 2021-10-25T11:19:44Z

internal/files/compress.go

+		ImplicitTopLevelFolder: false,
+	}
+
+	// Create a temporary work directory to properly name the root directory in the archive, e.g. aws-1.0.1


I used the temporary folder with proper name to create the .zip package with root <packageName>-<version>. It's a workaround for mholt/archiver, which doesn't support it.

If you think that copy to temp is an overkill, I will have to implement our own simple zip archiver.

SGTM, we can revisit it later if problems are found.

jsoriano · 2021-10-25T12:49:52Z

I think we need to be consistent with current .zip files, for example: https://epr.elastic.co/epr/aws/aws-1.1.0.zip , so I will adjust the logic here :)

Ah yes, this is a stronger point 🙂

(Btw, side thoughts now, having the package assets inside a directory might allow to embed in the zip other validation or control data, like a signature slightly_smiling_face)

So with this approach we agree on the content addressed signatures. WDYT?

This would be some kind of content addressed signatures, yes. The good point is that everything would be contained inside the package, we wouldn't need an extra file for the signature. Do you have any preference?

mtojek · 2021-10-25T12:52:13Z

To be honest I prefer the extra file as it's more intuitive :) Take the .zip file, sign it and have a signature. Frankly speaking we need to decide about this now, as it impacts both PRs: this and EPR.

jsoriano · 2021-10-25T12:32:21Z

internal/files/compress.go

+	tempDir := filepath.Join(os.TempDir(), folderNameFromFileName(destinationFile))
+	os.MkdirAll(tempDir, 0755)
+	defer os.RemoveAll(tempDir)


Consider using os.MkdirTemp() to be sure that different executions don't affect each other.

Suggested change

tempDir := filepath.Join(os.TempDir(), folderNameFromFileName(destinationFile))

os.MkdirAll(tempDir, 0755)

defer os.RemoveAll(tempDir)

tmpBaseDir, err := os.MkdirTemp("", "elastic-package-")

if err != nil { ... }

defer os.RemoveAll(tmpBaseDir)

tempDir := filepath.Join(tmpBaseDir, folderNameFromFileName(destinationFile))

os.MkdirAll(tempDir, 0755)

I can try it, but have to preserve the last element (for example: aws-1.0.1).

jsoriano · 2021-10-25T12:36:25Z

internal/files/compress.go

+		ImplicitTopLevelFolder: false,
+	}
+
+	// Create a temporary work directory to properly name the root directory in the archive, e.g. aws-1.0.1


SGTM, we can revisit it later if problems are found.

jsoriano · 2021-10-25T12:56:43Z

To be honest I prefer the extra file as it's more intuitive :) Take the .zip file, sign it and have a signature. Frankly speaking we need to decide about this now, as it impacts both PRs: this and EPR.

Ok, let's go with the extra file.

jsoriano · 2021-10-25T13:27:27Z

internal/files/compress.go

+	}
+	workDir := filepath.Join(tempDir, folderNameFromFileName(destinationFile))
+	os.MkdirAll(workDir, 0755)
+	defer os.RemoveAll(tempDir)


This defer should be immediately after the if err.... If at some moment we handle the error returned by MkdirAll and return there we may forget of removing the temp dir.

Thanks, fixed

mtojek added 8 commits October 21, 2021 15:00

Create stub for creating zipped package

c1cb8bc

Fix

ecc1cc0

Use z.Archive

d450981

Debug calls

90d3b15

os.Remove

3dd4b2e

Fix

51d698f

CI: introduce build-zip goal

27fa9c9

Implement clean

255b64e

mtojek self-assigned this Oct 21, 2021

mtojek added 4 commits October 21, 2021 18:26

Fix

44fa6af

Don't update the stack

e652936

Remove build directories

85d4a4e

Install zip packages

1bd04c7

mtojek requested a review from jsoriano October 21, 2021 16:56

mtojek marked this pull request as draft October 21, 2021 16:56

mtojek added 2 commits October 25, 2021 12:20

WIP

89222b0

Add missing root directory in the zip

a013376

mtojek commented Oct 25, 2021

View reviewed changes

mtojek added 2 commits October 25, 2021 13:35

Fix

0ccb162

We need Kibana too

5c4ca2b

jsoriano reviewed Oct 25, 2021

View reviewed changes

Address PR comments

a4e8466

mtojek marked this pull request as ready for review October 25, 2021 13:19

jsoriano reviewed Oct 25, 2021

View reviewed changes

Fix

92ef5d7

jsoriano approved these changes Oct 25, 2021

View reviewed changes

mtojek mentioned this pull request Oct 25, 2021

Proposal for cryptographically signing integration packages elastic/package-registry#728

Closed

mtojek merged commit 4e7ab31 into elastic:master Oct 25, 2021

jsoriano mentioned this pull request Feb 22, 2022

[Change Proposal] Remove release tag from packages elastic/package-spec#225

Closed

14 tasks

Conversation

mtojek commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

mtojek commented Oct 21, 2021

Uh oh!

jsoriano commented Oct 25, 2021

Uh oh!

mtojek commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

jsoriano Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

jsoriano commented Oct 25, 2021

Uh oh!

mtojek commented Oct 25, 2021

Uh oh!

jsoriano Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

mtojek Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

jsoriano Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

jsoriano commented Oct 25, 2021

Uh oh!

jsoriano Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

mtojek Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtojek commented Oct 21, 2021 •

edited

Loading

elasticmachine commented Oct 21, 2021 •

edited

Loading

mtojek commented Oct 25, 2021 •

edited

Loading